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

mapbox/driving-traffic support added to the list of directions profil… #292

Merged
merged 3 commits into from
Jan 31, 2017

Conversation

srabenja
Copy link
Contributor

…es, and checks added for maximum number of coordinates that can be provided, as discussed with @zugaldia

…es; checks added for maximum number of coordinates provided
Copy link
Member

@zugaldia zugaldia left a comment

Choose a reason for hiding this comment

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

I think the traffic response is similar to the non-traffic response, still, and in case this changes in the future, how about adding a test like mentioned on #290 (comment)?

@@ -464,6 +465,18 @@ public MapboxDirections build() throws ServicesException {
"You should provide at least two coordinates (from/to).");
}


if (profile != null && profile.equals(DirectionsCriteria.PROFILE_DRIVING_TRAFFIC)
Copy link
Member

@zugaldia zugaldia Jan 27, 2017

Choose a reason for hiding this comment

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

@srabenja Considering that profile and coordinates are required parameters, how about we add a check before these two to raise an exception if either is null (or empty)?

@srabenja
Copy link
Contributor Author

srabenja commented Jan 27, 2017

@zugaldia I added the changes you mentioned
it also fixes the lack of a test for > 25 coordinates

@zugaldia
Copy link
Member

@srabenja I see the fixture, but I see no tests using it?

@cammace
Copy link

cammace commented Jan 30, 2017

I have fixed a few documentation things, cleaned up the fixture, rewrote some exception messages to make it more clear what error occurred and lastly, added a few test. @zugaldia can you review this again?

@cammace cammace merged commit 89a6b54 into master Jan 31, 2017
@cammace cammace deleted the 290-driving-traffic-support branch January 31, 2017 16:12
@zugaldia zugaldia mentioned this pull request Feb 10, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Feb 22, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 9, 2017
9 tasks
@zugaldia zugaldia mentioned this pull request Mar 17, 2017
9 tasks
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.

3 participants