Skip to content

Fix waypoints APIs naming for map matching and directions services#962

Merged
Guardiola31337 merged 1 commit into
masterfrom
pg-align-waypoints-apis-naming
Feb 12, 2019
Merged

Fix waypoints APIs naming for map matching and directions services#962
Guardiola31337 merged 1 commit into
masterfrom
pg-align-waypoints-apis-naming

Conversation

@Guardiola31337

@Guardiola31337 Guardiola31337 commented Feb 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Follow up PR from latest feedback in #961

Edited:

Noting that for the Map Matching service we're deprecating

mapbox-java/services-matching/src/main/java/com/mapbox/api/matching/v5/MapboxMapMatching.java

Line 488 in c834c07

public Builder waypoints(@Nullable @IntRange(from = 0) Integer... waypoints) {
in favor of

mapbox-java/services-matching/src/main/java/com/mapbox/api/matching/v5/MapboxMapMatching.java

Line 507 in aba088c

public Builder waypointIndices(@Nullable @IntRange(from = 0) Integer... waypointIndices) {
to avoid breaking SemVer.

cc @1ec5

@Guardiola31337 Guardiola31337 self-assigned this Feb 11, 2019
@Guardiola31337 Guardiola31337 force-pushed the pg-align-waypoints-apis-naming branch from 23a1d74 to c6dce90 Compare February 12, 2019 05:03
danesfeder
danesfeder reviewed Feb 12, 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.

@Guardiola31337 left some comments regarding naming / javadoc. We are close, but imo this is still a little confusing for developers looking at the documentation for the first time. cc @1ec5

Comment thread services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java
* Optionally, set which input coordinates should be treated as via way points.
* Optionally, set which input coordinates should be treated as separate legs.
* <p>
* Most useful in combination with steps=true and requests based on traces

@danesfeder danesfeder Feb 12, 2019

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.

Does traces make sense here? Reading this javadoc, I'm still not entirely sure what separateLegs means.

set which input coordinates should be treated as separate legs.

By adding coordinates to a Directions API request, you are creating new legs. I think it may make sense here to say that separateLegs is directly influencing the guidance behavior when set.

@Guardiola31337 Guardiola31337 Feb 12, 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.

#962 (comment)

@Guardiola31337 Guardiola31337 force-pushed the pg-align-waypoints-apis-naming branch from c6dce90 to aba088c Compare February 12, 2019 15:30
@Guardiola31337

Guardiola31337 commented Feb 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

After some discussion, we agreed on fixing the naming to "waypoint indices":

Hope this is clear now.

cc @osana @1ec5 @mcwhittemore

@Guardiola31337

Guardiola31337 commented Feb 12, 2019

Copy link
Copy Markdown
Contributor Author
  • In the Directions service the indices added mean which waypoints should be "silent" (from a guidance point of view). Note the explicit silent naming used here.

Actually, this is not technically correct because coordinate indices not added act as silent waypoints, so not adding them is what makes them silent.

I'm updating (last time, promise!) the naming to waypointIndices and adding a note to Javadoc so it makes total sense to developers using the services. FWIW both Map Matching and Directions services will use the same nomenclature which was the initial goal of this PR.

Sorry for the back and forth.

@Guardiola31337 Guardiola31337 force-pushed the pg-align-waypoints-apis-naming branch from c807d61 to 2a5e46c Compare February 12, 2019 17:50
danesfeder
danesfeder approved these changes Feb 12, 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 the updates @Guardiola31337 this is much more clear

@Guardiola31337 Guardiola31337 merged commit 9d971f7 into master Feb 12, 2019
@Guardiola31337 Guardiola31337 deleted the pg-align-waypoints-apis-naming branch February 12, 2019 17:54
@langsmith

langsmith commented Feb 12, 2019

Copy link
Copy Markdown

@danesfeder & @Guardiola31337 , if needed, please don't forget about updating mentions of waypoints in https://docs.mapbox.com/android/java/overview/directions/ and/or https://docs.mapbox.com/android/java/overview/map-matching/

thanks

cc @colleenmcginnis

@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

1 more reviewer

@danesfeder danesfeder danesfeder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Guardiola31337 Guardiola31337

Labels

Directions Map Matching Ready For Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Guardiola31337 @langsmith @danesfeder