Skip to content

Conversation

@Guardiola31337
Copy link
Contributor

@Guardiola31337 Guardiola31337 commented Feb 11, 2019

Follow up PR from latest feedback in #961

Edited:

Noting that for the Map Matching service we're deprecating

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

cc @1ec5

Copy link
Contributor

@danesfeder danesfeder left a comment

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

* 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
Copy link
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Guardiola31337 commented Feb 12, 2019

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

Hope this is clear now.

cc @osana @1ec5 @mcwhittemore

@Guardiola31337
Copy link
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
Copy link
Contributor

@danesfeder danesfeder left a comment

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
Copy link

@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
14 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