Convert FIRGetOptions.initWithSource to factory method. (#655)

[rsgowman]

No description provided.

[wilhuff]

Methods should lead with their return type in Objective-C

This should be optionsWithSource:

[rsgowman]

Done

[ryanwilson]

Sorry about the delay in review!

[ryanwilson]

I think leaving this as a constructor would be good instead of fighting against the translator: GetOptions(source: .cache).

[wilhuff]

So the issue is that we're going to add a timeout too.

In the other languages you'll be able to do things like this:

GetOptions.source(GetOptions.Source.CACHE).timeout(2, TimeUnit.MINUTES);

GetOptions.timeout(2, TimeUnit.MINUTES).source(GetOptions.Source.CACHE);

Making this an initializer only thing means that the same flexibility would require us to define all the permutations of options which doesn't seem like what we want to do at all.

[ryanwilson]

Ahhh okay, that makes sense. This wouldn't solve it entirely though, would it? You'd be left with an instance instead and unable to call the next class method. Unless each option was a constructor and a function:

class func source(_ source: GetSource) -> GetOptions
class func timeout(_ value: Int, _ unit: TimeUnit) -> GetOptions

func source(_ source: GetSource) -> GetOptions
func timeout(_ value: Int, _ unit: TimeUnit) -> GetOptions

Do you mean to have this as an instance method, or am I missing something?

It's worth noting that this could potentially go away once we have the mechanism to ship native Swift files:

public func init(source: GetOptions.Source = .default, 
                 timeout: (Int, TimeUnit) = (2, .minutes)) 
{                   // (or have Timeout as its own type)
  // Implementation
}

// Usage:
GetOptions()
GetOptions(source: .cache)
GetOptions(timeout: (5, .seconds))
GetOptions(source: .cache, timeout: (8, .seconds))

[rsgowman]

Yeah, I think the idea would be to add instance methods as appropriate to allow chaining.

I like the idea of (eventually) using named parameters. (There's a concern if we ever have a very large number of options, but I don't think that's the case here.) Is there an approx timeline for being able to do this?

[ryanwilson]

I'm fine with the API with one minor change to match SnapshotOptions: remove the argument label to make it source(_:), which turns the usage into: GetSource.source(.cache). I would be interested to see how it's going to expand with multiple parameters though - class and instance functions?

[rsgowman]

Done; ptal.

(And yeah; expected path forward is to expand with class and instance functions.)