Added wrappers for route tile APIs #913
Conversation
devotaaabel
requested review from
zugaldia,
Guardiola31337,
danesfeder and
osana
devotaaabel
changed the title
devotaaabel
force-pushed
the
devota-add-route-tiles-service
branch
3 times, most recently
from
ea1541b
to
86d491f
Compare
There was a problem hiding this comment.
@devotaaabel this looks great overall, just a couple typos 👍
I know you mentioned you're still working through some tests - we should add some that ensure the response model are properly generated. We do this with the Directions API to ensure everything is being deserialized correctly.
Going to leave the final approval to @osana
services-route-tiles/src/main/java/com/mapbox/api/routetiles/v1/package-info.java
Outdated
Show resolved
Hide resolved
...ces-route-tiles/src/main/java/com/mapbox/api/routetiles/v1/versions/models/package-info.java
Outdated
Show resolved
Hide resolved
devotaaabel
force-pushed
the
devota-add-route-tiles-service
branch
5 times, most recently
from
d1ce2b1
to
3c7de52
Compare
| * {@link MapboxRouteTiles.Builder}. | ||
| * | ||
| * @param userAgent the user agent | ||
| * @param coordinates a string value of the min and max longitude and latitude |
There was a problem hiding this comment.
we should specify the order and what separator is used
| * @since 4.1.0 | ||
| */ | ||
| @AutoValue | ||
| public abstract class RouteTileVersionsResponse { |
There was a problem hiding this comment.
I believe it should implement Serializable
There was a problem hiding this comment.
@osana in what case would it make sense to implement serializable? The gson TypeAdapterFactory should take care of the conversion to this object from the json response, right?
There was a problem hiding this comment.
@devotaaabel thoughout mapbox-java services we support the use of serialize() and deserialize() and serialize()
It can be ticketed and fixed in another pr
There was a problem hiding this comment.
ticketed here: #917
| * @return this builder for chaining options together | ||
| * @since 4.1.0 | ||
| */ | ||
| public abstract Builder boundingBox(@NonNull BoundingBox boundingBox); |
There was a problem hiding this comment.
It would be nice to have a version that take (minLon, minLat, maxLon, maxLat)
There was a problem hiding this comment.
@osana a bounding box can be created with BoundingBox.fromLngLats, is that sufficient?
There was a problem hiding this comment.
I think it is a helper method so that users did not have to deal with it
I do not have a strong preference about it. Either way is fine.
One big question will be what happens over date line!
devotaaabel
force-pushed
the
devota-add-route-tiles-service
branch
2 times, most recently
from
5d03217
to
b0fecfa
Compare
devotaaabel
force-pushed
the
devota-add-route-tiles-service
branch
from
b0fecfa
to
e438e67
Compare
Closes #912.
This PR adds a new services module,
services-route-tiles, which includes the following endpoint wrapper classes:MapboxRouteTilesMapboxRouteTileVersions.Please see the route tiles API readme for explanation of the two endpoints.
I'm currently trying to add more meaningful tests, specifically to test the json conversion.