Provide convenience initializers for options #685
Conversation
| /** | ||
| Initializes an options object for routes between the given waypoints and an optional profile identifier. | ||
|
|
||
| Do not call `DirectionsOptions(waypoints:profileIdentifier:)` directly; instead call the corresponding initializer of `RouteOptions` or `MatchOptions`. |
There was a problem hiding this comment.
Since developers should not call DirectionsOptions(waypoints:profileIdentifier:) directly, is it safe to not include this change?
There was a problem hiding this comment.
We should include it, because it is valid, if somewhat contrived, to subclass DirectionsOptions directly. The use case would be building support for another navigation API service endpoint. Normally that’s something this library would have built right in, but in principle, client code could implement that support all the same.
| /** | ||
| Initializes an options object for routes between the given waypoints and an optional profile identifier. | ||
|
|
||
| Do not call `DirectionsOptions(waypoints:profileIdentifier:)` directly; instead call the corresponding initializer of `RouteOptions` or `MatchOptions`. |
There was a problem hiding this comment.
We should include it, because it is valid, if somewhat contrived, to subclass DirectionsOptions directly. The use case would be building support for another navigation API service endpoint. Normally that’s something this library would have built right in, but in principle, client code could implement that support all the same.
| - parameter waypoints: An array of `Waypoint` objects representing locations that the route should visit in chronological order. The array should contain at least two waypoints (the source and destination) and at most 25 waypoints. (Some profiles, such as `ProfileIdentifier.automobileAvoidingTraffic`, [may have lower limits](https://www.mapbox.com/api-documentation/#directions).) | ||
| - parameter profileIdentifier: A string specifying the primary mode of transportation for the routes. `ProfileIdentifier.automobile` is used by default. | ||
| */ | ||
| public convenience init(waypoints: [Waypoint], profileIdentifier: ProfileIdentifier? = nil) { |
There was a problem hiding this comment.
Let’s add a test that subclasses RouteOptions and overrides just this initializer, not init(waypoints:profileIdentifier:queryItems:). It doesn’t have to do anything; we just need to see if it would compile. I’m concerned that the required bit means it still needs to be implemented even if the convenience initializer would call the required one.
There was a problem hiding this comment.
Is it sufficient to subclass RouteOptions, override the convenience init, and initialize a RouteOptionsSubclass object through the override?
|
|
||
| class testRouteOptionsSubclass: RouteOptions { | ||
| convenience init(waypoints: [Waypoint], profileIdentifier: ProfileIdentifier?) { | ||
| self.init(waypoints: [], profileIdentifier: nil, queryItems: nil) |
There was a problem hiding this comment.
Can you try to match what NavigationRouteOptions was doing here more closely, so we can be sure the incompatibility is fixed?
Alternatively, you could try checking out navigation SDK v2.3.x and pointing it to this branch to see if it’ll compile. Thanks!
This PR fixes #684 by adding convenience initializers for
RouteOptions,MatchOptions, andDirectionsOptions.