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

Directions Refresh new implementation #955

Merged
merged 17 commits into from
Feb 12, 2019

Conversation

devotaaabel
Copy link
Contributor

@devotaaabel devotaaabel commented Jan 28, 2019

This PR creates the infrastructure to allow updating routes with updated annotations via the directions-refresh endpoint (This endpoint is currently only on staging). Included in this PR:

  • A directions request can contain a new boolean parameter enable_refresh. If this parameter is set to true, the route will be cached and will be refreshable via the directions-refresh endpoint.

  • The directions-refresh request needs to include the route_id, route_index, leg_index, access_token, and whatever annotations needed.

  • The directions-refresh response is basically a barebones DirectionsRoute, which only includes a list of barebones RouteLegs, which only include the requested LegAnnotations.

Todo:

  • Add enable_refresh option to directions API
  • Create DirectionsRefresh service module
  • Create sample test
  • Create unit test
  • Add javadoc

@devotaaabel devotaaabel changed the title [WIP] Directions Refresh new implementation Directions Refresh new implementation Feb 11, 2019
@devotaaabel devotaaabel removed the WIP label Feb 11, 2019
@devotaaabel devotaaabel force-pushed the devota-directions-refresh-two branch from fa6d4e1 to 9a320a9 Compare February 11, 2019 21:38
@devotaaabel devotaaabel force-pushed the devota-directions-refresh-two branch from c0d0fd0 to 9fb2ef2 Compare February 11, 2019 21:50
@devotaaabel devotaaabel force-pushed the devota-directions-refresh-two branch from 9fb2ef2 to 8aceed9 Compare February 11, 2019 21:50
@devotaaabel
Copy link
Contributor Author

@Guardiola31337 @danesfeder this is ready for review. The sample test runs properly when adjusted to run on staging. See todos for what I'm still working on

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.

This is looking great, thanks for the work keeping this moving with @gknisely.

The directions-refresh response is basically a barebones DirectionsRoute, which only includes a list of barebones RouteLegs, which only include the requested LegAnnotations.

My only question is regarding this - if we are only getting a List of LegAnnotation, does it make sense to return just that? If that's the case, I think it would be more clear to developers using the response data.

public static Builder builder() {
return new AutoValue_MapboxDirectionsRefresh.Builder()
.baseUrl(Constants.BASE_API_URL)
.routeIndex(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting a compilation error when running this because route / leg index are defined as Strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danesfeder this is updated now. Also it's running on production now so the sample test should pass

@langsmith
Copy link

@devotaaabel & @danesfeder , if needed, please don't forget about updating content on https://docs.mapbox.com/android/java/overview/directions given this pr's changes.

thank you

cc @colleenmcginnis

public abstract Builder clientAppName(@NonNull String clientAppName);

/**
* Optionally change the APIs base URL to something other then the default Mapbox one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo other *then

@devotaaabel devotaaabel force-pushed the devota-directions-refresh-two branch from 2e1fbba to 9085c51 Compare February 12, 2019 20:57
@devotaaabel devotaaabel force-pushed the devota-directions-refresh-two branch from 9085c51 to ad63326 Compare February 12, 2019 21:08
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.

@devotaaabel great work here - let's do one last sanity check with the updated v1 endpoint before we merge. Otherwise, this is good to go ✅

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

let's do one last sanity check with the updated v1 endpoint before we merge

I've just run the BasicDirectionsRefresh sample and it worked so we're good here 🚀

Great work @devotaaabel :shipit:

@devotaaabel devotaaabel merged commit f7a798d into master Feb 12, 2019
@devotaaabel devotaaabel deleted the devota-directions-refresh-two branch February 12, 2019 22:35
@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.

4 participants