Skip to content

Directions Refresh new implementation#955

Merged
devotaaabel merged 17 commits into
masterfrom
devota-directions-refresh-two
Feb 12, 2019
Merged

Directions Refresh new implementation#955
devotaaabel merged 17 commits into
masterfrom
devota-directions-refresh-two

Conversation

@devotaaabel

@devotaaabel devotaaabel commented Jan 28, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 self-assigned this Jan 28, 2019
@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

devotaaabel commented Feb 11, 2019

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

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.

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.

Comment thread ...tions-refresh/src/main/java/com/mapbox/api/directionsrefresh/v1/MapboxDirectionsRefresh.java Outdated
public static Builder builder() {
return new AutoValue_MapboxDirectionsRefresh.Builder()
.baseUrl(Constants.BASE_API_URL)
.routeIndex(0)

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

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

@devotaaabel devotaaabel 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.

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

@devotaaabel devotaaabel force-pushed the devota-directions-refresh-two branch from f3cf149 to bc6d50e Compare February 12, 2019 16:28
@langsmith

langsmith commented Feb 12, 2019

Copy link
Copy Markdown

@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

Guardiola31337
Guardiola31337 reviewed Feb 12, 2019
View reviewed changes
Comment thread ...tions-refresh/src/main/java/com/mapbox/api/directionsrefresh/v5/MapboxDirectionsRefresh.java
public abstract Builder clientAppName(@NonNull String clientAppName);

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

@Guardiola31337 Guardiola31337 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.

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
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.

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

Guardiola31337
Guardiola31337 approved these changes Feb 12, 2019
View reviewed changes

@Guardiola31337 Guardiola31337 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.

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

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

@danesfeder danesfeder danesfeder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@devotaaabel devotaaabel

Labels

Directions Feature Ready For Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@devotaaabel @langsmith @Guardiola31337 @danesfeder