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

Parse destinations #63

Merged
merged 3 commits into from
Aug 2, 2016
Merged

Parse destinations #63

merged 3 commits into from
Aug 2, 2016

Conversation

bsudekum
Copy link

@bsudekum bsudekum commented Aug 1, 2016

Closes: #62

@1ec5 Would it be worth changing the test case that uses a destination? Am I missing anything else?

@@ -506,10 +506,17 @@ public class RouteStep: NSObject {
*/
public let transportType: TransportType?


/**
Destinations signs for the current way.
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation is written for an audience that may not be familiar with routers. How about something like this:

Destinations, such as control cities, that appear on guide signage for the road identified in the name property.

This property is typically available in steps leading to or from a freeway or expressway.

@1ec5
Copy link
Contributor

1ec5 commented Aug 1, 2016

Would it be worth changing the test case that uses a destination? Am I missing anything else?

Please feel free to add additional test cases. We’ve been wanting to do that for a while (e.g., #31).

@1ec5 1ec5 added the improvement Improvement for an existing feature. label Aug 1, 2016
@bsudekum
Copy link
Author

bsudekum commented Aug 1, 2016

Updated description and added destinations test.


This property is typically available in steps leading to or from a freeway or expressway.
*/
public let destinations: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Developers will expect this property to be an array if we name it destinations, plural. I think it could be called destination, by analogy with name, which is singular despite the fact that it can sometimes contain multiple names in one string.

One thing I'm unsure about is whether it's a problem that "destination" already conceptually refers to the destination waypoint. But let's go with destination and revisit if there's any confusion. The documentation is pretty clear in any case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, then again, we have instructions, plural. Alright, your call. 😅

Copy link
Author

Choose a reason for hiding this comment

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

Always a fan of parity, I'd like to leave as is.

@bsudekum bsudekum merged commit a6a3cb6 into master Aug 2, 2016
@bsudekum bsudekum deleted the destination branch August 2, 2016 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants