Skip to content

Fix via waypoints APIs naming#961

Merged
Guardiola31337 merged 1 commit into
masterfrom
pg-fix-via-waypoints-apis-naming
Feb 11, 2019
Merged

Fix via waypoints APIs naming#961
Guardiola31337 merged 1 commit into
masterfrom
pg-fix-via-waypoints-apis-naming

Conversation

@Guardiola31337

@Guardiola31337 Guardiola31337 commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

We found in mapbox/mapbox-navigation-android#1733 (comment) that naming could be confusing and should be more explicit in order to make clients' life easier.

@Guardiola31337 Guardiola31337 self-assigned this Feb 11, 2019
danesfeder
danesfeder approved these changes Feb 11, 2019
View reviewed changes

@danesfeder danesfeder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this update @Guardiola31337

@Guardiola31337 Guardiola31337 merged commit 6e6110a into master Feb 11, 2019
@Guardiola31337 Guardiola31337 deleted the pg-fix-via-waypoints-apis-naming branch February 11, 2019 16:11
@osana

osana commented Feb 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@Guardiola31337
If we are doing this name change, we should do a similar name change in MapboxMapMatching
I guess we can deprecate the use of waypoints and add viawaypoints (for now)

Also please add viawaypoints to RouteOptions

akitchen
akitchen reviewed Feb 11, 2019
View reviewed changes
Comment thread services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java
private Point destination;
private Point origin;
private String[] approaches;
private Integer[] waypoints;

@akitchen akitchen Feb 11, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was correct as-was. Should be waypoints as one word, no capital P

@Guardiola31337 Guardiola31337 Feb 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're correct. Will update in a follow-up PR. Thanks for flagging 🙇

@Guardiola31337

Guardiola31337 commented Feb 11, 2019

Copy link
Copy Markdown
Contributor Author

@osana

If we are doing this name change, we should do a similar name change in MapboxMapMatching
I guess we can deprecate the use of waypoints and add viawaypoints (for now)

Yeah, I agree. We should be consistent across the different services. Will implement ☝️ if we're not cool breaking SemVer.

Also please add viawaypoints to RouteOptions

You mean, adding "waypoints" option to Map Matching route options, right? I've already added it to Directions 👀

mapbox-java/services-directions/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

Line 284 in 57e8710

public abstract String viaWayPoints();

mapbox-java/services-directions/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

Line 569 in 57e8710

public abstract Builder viaWayPoints(@Nullable String indices);

@osana

osana commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

@Guardiola31337 Please add a call to viaWaypoints() when RouteOptions are being saved/generated:

mapbox-java/services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java

Line 235 in 6e6110a

RouteOptions.builder()

I believe tests might be updated as well.

@Guardiola31337 Guardiola31337 mentioned this pull request Feb 11, 2019
Merged
@osana osana mentioned this pull request Feb 12, 2019
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@osana osana Awaiting requested review from osana

2 more reviewers

@akitchen akitchen akitchen left review comments

@danesfeder danesfeder danesfeder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Guardiola31337 Guardiola31337

Labels

Directions Ready For Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Guardiola31337 @osana @akitchen @danesfeder