Skip to content

Introducing Geocoded Waypoints into Directions API result.#118

Merged
samthor merged 9 commits intogooglemaps:masterfrom
domesticmouse:master
Dec 7, 2015
Merged

Introducing Geocoded Waypoints into Directions API result.#118
samthor merged 9 commits intogooglemaps:masterfrom
domesticmouse:master

Conversation

@domesticmouse
Copy link
Copy Markdown
Contributor

I had to break API backward compatibility with this PR, as we were previously returning an array.

I had to break API backward compatibility with this API, as we were previously returning an array. Bad life choices.
@domesticmouse
Copy link
Copy Markdown
Contributor Author

PTAL @markmcd

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.

DirectionsApi.getDirections(...) is a nice shorthand that lets a user jump straight to the data, compared with newRequest(..) that let's them tweak more things.

Can you restrict the break to just DirectionsApi.newRequest(..) instead? e.g. by introducing a FullDirectionsApiRequest subclass?

There's likely be some tradeoffs we need to make, so could warrant a quick chat IRL with the team.

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.

Introducing a full results vs an elided just the routes result was a reasonably light change. WDYT?

@domesticmouse
Copy link
Copy Markdown
Contributor Author

PTAL @markmcd =)

@domesticmouse
Copy link
Copy Markdown
Contributor Author

And @samthor

@markmcd
Copy link
Copy Markdown
Contributor

markmcd commented Dec 6, 2015

Looks good! It might be nice to get another pair of eyes over this for sanity's sake, but I wouldn't consider it blocking.

LGTM

@domesticmouse
Copy link
Copy Markdown
Contributor Author

Ping @samthor =)

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.

For the full request, use {@link Full...}

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.

Added commentary to both DirectionsApiRequest.java and FullDirectionsApiRequest.java

@domesticmouse
Copy link
Copy Markdown
Contributor Author

PTAL @samthor - landed the Directions API simplification.

@samthor
Copy link
Copy Markdown
Contributor

samthor commented Dec 7, 2015

LGTM

samthor added a commit that referenced this pull request Dec 7, 2015
Introducing Geocoded Waypoints into Directions API result.
@samthor samthor merged commit d54de1a into googlemaps:master Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants