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

Waypoint targets #326

Merged
merged 2 commits into from Dec 14, 2018
Merged

Waypoint targets #326

merged 2 commits into from Dec 14, 2018

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Dec 12, 2018

Added the Waypoint.targetCoordinate property corresponding to the Directions API’s waypoint_targets property, which specifies a more specific destination location for arrival instructions.

Fixed an issue where the Waypoint.allowsArrivingOnOppositeSide property was not copied when copying a Waypoint object. Added unit tests of copying and coding waypoints, as well as specifying targetCoordinate and other waypoint properties in conjunction with a RouteOptions object.

Added documentation associating some properties with their API counterparts for discoverability.

/cc @mapbox/navigation-ios @danesfeder @danpaz

@1ec5 1ec5 added bug improvement Improvement for an existing feature. labels Dec 12, 2018
@1ec5 1ec5 self-assigned this Dec 12, 2018
@1ec5 1ec5 requested a review from frederoni December 12, 2018 00:57
@@ -328,7 +328,7 @@ open class DirectionsOptions: NSObject, NSSecureCoding, NSCopying {
An array of directions query strings to include in the request URL.
*/
internal var queries: [String] {
return waypoints.compactMap { return "\($0.coordinate.longitude.rounded(to: 1e6)),\($0.coordinate.latitude.rounded(to: 1e6))" }
return waypoints.compactMap{ $0.coordinate.stringForRequestURL }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the following edge case: if the developer specifies three waypoints and one of them is invalid, this library previously sent a request with all three waypoints, all but guaranteeing an error in the response. Now it sends only two of the waypoints, which may return a valid response (ignoring the invalid coordinate). However, if the invalid waypoint had the name or targetCoordinate property set, then there would be a mismatch between the path and query parameters in the request URL, resulting in a different error than before in the response.

Added the Waypoint.targetCoordinate property corresponding to the Directions API’s waypoint_targets property. Added unit tests of copying and coding waypoints, as well as specifying waypoint_targets and other waypoint properties in conjunction with a RouteOptions object. Added documentation associating some properties with their API counterparts for discoverability.
@1ec5
Copy link
Contributor Author

1ec5 commented Dec 14, 2018

Tests were failing because of a broken file reference; fixed in 6a2a707.

@1ec5 1ec5 merged commit b07c8e3 into master Dec 14, 2018
@1ec5 1ec5 deleted the 1ec5-waypoint-targets branch December 14, 2018 20:36
@1ec5 1ec5 added this to the v0.26.0 milestone Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug improvement Improvement for an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants