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

Port OSRM Text Instructions to v0.5.0 #26

Merged
merged 4 commits into from
Jul 7, 2017
Merged

Port OSRM Text Instructions to v0.5.0 #26

merged 4 commits into from
Jul 7, 2017

Conversation

1ec5
Copy link
Member

@1ec5 1ec5 commented Jul 6, 2017

This PR gets the Swift library back in sync with the JavaScript library, upgrading it from v0.2.1 to Project-OSRM/osrm-text-instructions@020e28b, which is equivalent to v0.4.0 plus Project-OSRM/osrm-text-instructions#116 and some translation updates. To wit, this PR makes the following changes:

(Project-OSRM/osrm-text-instructions#107 is irrelevant to this library because Bundle handles language fallbacks in Swift.)

Depends on mapbox/mapbox-directions-swift#147, which is on hold until the exits property lands in the production Directions API.

/cc @bsudekum @freenerd @willwhite

case "exit_number":
type = .exit
type = .exitIndex
Copy link
Member

Choose a reason for hiding this comment

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

why not stick with the js names exit and exitNumber?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reasoning as in mapbox/mapbox-directions-swift#147, where exitCodes is for what’s colloquially called an exit number (exitIdentifiers being another candidate) and exitIndex remains the count of exits from the beginning of the step. I figured consistency with MapboxDirections.swift would be more important for Swift developers than consistency with the JavaScript library they’ll never use. But if you think it’s more important to keep the libraries consistent with each other (to make merging more straightforward?), I can change them to exit and exitNumber here.

Copy link
Member Author

@1ec5 1ec5 Jul 7, 2017

Choose a reason for hiding this comment

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

Per discussion with @bsudekum, I’m going to merge this and get a release out in order to unblock a navigation SDK release. However, if you think we should change the enumeration values back to match the JavaScript version, please open a ticket and we can address it in a subsequent release. (We’re pre-1.0, so we don’t have to worry much about backwards compatibility.)

1ec5 added 2 commits July 7, 2017 11:07
Added optional arguments to OSRMInstructionFormatter.string(for:modifyValueByKey:) to track which leg the instruction is on. When there are multiple waypoints in the route, indicate the ordinal number of the waypoint in the arrival instruction.

Fixed the raw value of TokenType.wayPoint.

Updated tests. Ensure that formatted ordinals match the fixture language (English) rather than the test machine’s language.
@1ec5 1ec5 changed the title Port OSRM Text Instructions post-v0.4.0 Port OSRM Text Instructions to v0.5.0 Jul 7, 2017
1ec5 added 2 commits July 7, 2017 13:54
Added TokenType.exitCode. Renamed TokenType.exit to exitIndex for clarity. Use the exit and exit_destination instructions if an exit code is present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include exit number in off-ramp instructions Count waypoint upon arrival
3 participants