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
Geocoding V6 preview #1550
Geocoding V6 preview #1550
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/geocoding-v6 #1550 +/- ##
==========================================================
+ Coverage 77.35% 77.66% +0.30%
- Complexity 968 1029 +61
==========================================================
Files 134 152 +18
Lines 4107 4343 +236
Branches 588 605 +17
==========================================================
+ Hits 3177 3373 +196
- Misses 679 707 +28
- Partials 251 263 +12
|
1839de2
to
63ab3cf
Compare
63ab3cf
to
eb5084d
Compare
...es-geocoding/src/main/java/com/mapbox/api/geocoding/v5/SingleElementSafeListTypeAdapter.java
Outdated
Show resolved
Hide resolved
* and batch geocoding. See derived classes for more information. | ||
* @param <T> response type. | ||
*/ | ||
public abstract class MapboxV6BaseGeocoding<T> extends MapboxService<T, V6GeocodingService> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have multiple options on how to distinguish v6 and v5 classes.
- Like you did: v6 package + v6 in the class names;
- Just the v6 package;
- New major services-geocoding release that would remove all v5 classes (for v5 use prev major release);
- maybe sth else?
What I don't like so much about option 1 is having v6 in class names (also in different places: sometimes at the beginning, sometimes in the middle). It just doesn't look right as part of the class name. Maybe it's subjective, idk.
I understand that with option 2 the user will be running into wrong imports, but there are not a lot of conflicting classes, right?
With option 3 it's not very convenient for us, because we'd have to support to 2 major trains and some of the modules are reused, so we can't just bump the major only for geocoding.
So I wonder why you chose this specific approach and which ones you've also considered. Also why not option 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising this question, I was going to discuss it because I don't like this option either.
- New major services-geocoding release that would remove all v5 classes (for v5 use prev major release);
v5 is not deprecated and I'm not sure if it's going to be, so it's not an option for now.
- Just the v6 package;
I like this option but I had the same concern as you described.
but there are not a lot of conflicting classes, right?
Yes, right. We can leave v6 prefix for entrypoint classes (V6ForwardGeocodingRequestOptions
, and MapboxV6BatchGeocoding
), and for few more cases like V6Context
, V6ContextElement
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v5 is not deprecated and I'm not sure if it's going to be, so it's not an option for now.
I'm not suggesting to deprecate it, rather use a different major of mapbox-java. However, having 2 active major trains is not a very good idea.
Yes, right. We can leave v6 prefix for entrypoint classes (V6ForwardGeocodingRequestOptions, and MapboxV6BatchGeocoding), and for few more cases like V6Context, V6ContextElement. What do you think?
Why these specific classes? What's the criteria? IMO that would be: MapboxV6Geocoding
, V6GeocodingService
, V6Context
, V6ContextElement
, V6Feature,
V6FeatureType`. These classes have conflicts with v5 and I added ContextElement and FeatureType for consistency (because we have Context and Feature).
public abstract class MapboxV6BaseGeocoding<T> extends MapboxService<T, V6GeocodingService> { | ||
|
||
@NonNull | ||
protected abstract String accessToken(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are all methods protected, but the base class is public? What are we actually exposing to the user with this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields are supposed to be set by a user via Builder class which is public. We use these fields later in sub classes when we make http request. It's like a public set-function and protected get function, but in this case set function is a builder class.
I think that in general we should keep everything non-public until proven otherwise, so they're with minimal possible visibility level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point and it makes sense.
However, I see multiple problems with this approach:
- Inconsistency: user created the object but can't "see" it: how will they test it, for example? They'd want something like: "when I receive this input data I create these options".
- User can't modify the existing object based on the actual data. You can imagine creating an interceptor that would, for example, check if
clientAppName
is null and, if it is, set it to some value.
* | ||
* @see <a href="https://docs.mapbox.com/api/search/geocoding-v6/#data-storage">Data storage</a> | ||
*/ | ||
public abstract T permanent(@NonNull Boolean permanent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the corresponding method in the MapboxV6BaseGeocoding
protected and here it's public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builder functions are public because they're supposed to be called by a user. See code example and this comment for more information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this thread is the same as #1550 (comment), let's continue there.
@Override | ||
protected Call<V6Response> initializeCall() { | ||
final V6RequestOptions requestOptions = requestOptions(); | ||
if (requestOptions instanceof V6ReverseGeocodingRequestOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these ifs if the behaviour is different? Can there be 2 classes? I mean split MapboxV6Geocoding
into MapboxV6ForwardGeocoding
and MapboxV6ReverseGeocoding
. These are different requests with different options, they don't have a lot in common, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference is request options, and the rest parts are the same. Actually, in the first iteration I made 2 separate classes MapboxV6ForwardGeocoding
and MapboxV6ReverseGeocoding
but found it unnecessary and overcomplicated. Users would have to work with two different APIs, we would have to double test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the only logic you have is getService().<name depending on options>(options)
. The only shared logic is getService()
. Or did I get it wrong?
services-geocoding/src/main/java/com/mapbox/api/geocoding/v6/models/V6Context.java
Show resolved
Hide resolved
services-geocoding/src/main/java/com/mapbox/api/geocoding/v6/models/V6Coordinates.java
Show resolved
Hide resolved
* Object containing coordinate parameters (lat, long) and accuracy. | ||
*/ | ||
@AutoValue | ||
public abstract class V6Coordinates implements Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need a builder for this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
services-geocoding/src/main/java/com/mapbox/api/geocoding/v6/models/V6MatchCode.java
Show resolved
Hide resolved
services-geocoding/src/main/java/com/mapbox/api/geocoding/v6/models/V6Properties.java
Show resolved
Hide resolved
c3816bc
to
345e23c
Compare
345e23c
to
255ffa3
Compare
Deferred, not in priority now. Will be reworked once backend in GA. |
Preview of the https://docs.mapbox.com/api/search/geocoding-v6/ in mapbox-java (note target branch is
feature
because V6 is still in private preview).