Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide convenience initializers for options #685

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changes to Mapbox Directions for Swift

## v2.4.1
* Fixed an issue that caused a compiler error when subclassing `RouteOptions`. ([#685](https://github.com/mapbox/mapbox-directions-swift/pull/685))

## v2.4.0

* Fixed a crash that occurred when `RouteOptions.roadClassesToAvoid` or `RouteOptions.roadClassesToAllow` properties contained multiple road classes.
Expand Down
12 changes: 12 additions & 0 deletions Sources/MapboxDirections/DirectionsOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,18 @@ open class DirectionsOptions: Codable {
}
}

/**
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`.
Copy link
Contributor Author

@jill-cardamon jill-cardamon Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since developers should not call DirectionsOptions(waypoints:profileIdentifier:) directly, is it safe to not include this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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://docs.mapbox.com/api/navigation/#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) {
self.init(waypoints: waypoints, profileIdentifier: profileIdentifier, queryItems: nil)
}

/**
Creates new options object by deserializing given `url`

Expand Down
10 changes: 10 additions & 0 deletions Sources/MapboxDirections/MapMatching/MatchOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ open class MatchOptions: DirectionsOptions {
self.init(waypoints: waypoints, profileIdentifier: profileIdentifier, queryItems: queryItems)
}

/**
Initializes a match options object for matching locations against the road network.

- 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://docs.mapbox.com/api/navigation/#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) {
self.init(waypoints: waypoints, profileIdentifier: profileIdentifier, queryItems: nil)
}

public required init(waypoints: [Waypoint], profileIdentifier: ProfileIdentifier? = nil, queryItems: [URLQueryItem]? = nil) {
super.init(waypoints: waypoints, profileIdentifier: profileIdentifier, queryItems: queryItems)

Expand Down
10 changes: 10 additions & 0 deletions Sources/MapboxDirections/RouteOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,16 @@ open class RouteOptions: DirectionsOptions {
self.arriveBy = arriveBy
}
}

/**
Initializes a route options object for routes between the given waypoints and an optional profile identifier.

- 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it sufficient to subclass RouteOptions, override the convenience init, and initialize a RouteOptionsSubclass object through the override?

self.init(waypoints: waypoints, profileIdentifier: profileIdentifier, queryItems: nil)
}

#if canImport(CoreLocation)
/**
Expand Down
8 changes: 8 additions & 0 deletions Tests/MapboxDirectionsTests/RouteOptionsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -363,3 +363,11 @@ var testRouteOptions: RouteOptions {

return opts
}

var routeOptionsSubclassTest = testRouteOptionsSubclass(waypoints: [])

class testRouteOptionsSubclass: RouteOptions {
convenience init(waypoints: [Waypoint], profileIdentifier: ProfileIdentifier?) {
self.init(waypoints: [], profileIdentifier: nil, queryItems: nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try to match what NavigationRouteOptions was doing here more closely, so we can be sure the incompatibility is fixed?

https://github.com/mapbox/mapbox-navigation-ios/blob/v2.3.1/Sources/MapboxCoreNavigation/NavigationRouteOptions.swift#L20

Alternatively, you could try checking out navigation SDK v2.3.x and pointing it to this branch to see if it’ll compile. Thanks!

}
}