Geocoding V6 preview by DzmitryFomchyn · Pull Request #1550 · mapbox/mapbox-java

DzmitryFomchyn · 2023-06-20T13:30:55Z

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

codecov · 2023-06-20T13:33:29Z

Codecov Report

Merging #1550 (255ffa3) into feature/geocoding-v6 (d69aa0e) will increase coverage by 0.30%.
The diff coverage is 83.05%.

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

Impacted Files	Coverage Δ
...geocoding/v5/SingleElementSafeListTypeAdapter.java	`82.35% <ø> (ø)`
...java/com/mapbox/api/geocoding/v6/models/Utils.java	`31.25% <31.25%> (ø)`
...om/mapbox/api/geocoding/v6/models/V6MatchCode.java	`42.85% <42.85%> (ø)`
...m/mapbox/api/geocoding/v6/models/V6Properties.java	`42.85% <42.85%> (ø)`
...apbox/api/geocoding/v6/V6StructuredInputQuery.java	`57.14% <57.14%> (ø)`
...m/mapbox/api/geocoding/v6/models/V6JsonObject.java	`75.00% <75.00%> (ø)`
.../com/mapbox/api/geocoding/v6/V6GeocodingUtils.java	`85.71% <85.71%> (ø)`
...com/mapbox/api/geocoding/v6/MapboxV6Geocoding.java	`88.05% <88.05%> (ø)`
...apbox/api/geocoding/v6/MapboxV6BatchGeocoding.java	`89.47% <89.47%> (ø)`
...geocoding/v6/V6ReverseGeocodingRequestOptions.java	`94.44% <94.44%> (ø)`
... and 9 more

dzinad · 2023-06-20T15:36:34Z

+ * and batch geocoding. See derived classes for more information.
+ * @param <T> response type.
+ */
+public abstract class MapboxV6BaseGeocoding<T> extends MapboxService<T, V6GeocodingService> {


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.

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?

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

dzinad · 2023-06-20T15:43:26Z

+public abstract class MapboxV6BaseGeocoding<T> extends MapboxService<T, V6GeocodingService> {
+
+  @NonNull
+  protected abstract String accessToken();


Why are all methods protected, but the base class is public? What are we actually exposing to the user with this approach?

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.

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.

dzinad · 2023-06-20T15:46:27Z

+     *
+     * @see <a href="https://docs.mapbox.com/api/search/geocoding-v6/#data-storage">Data storage</a>
+     */
+    public abstract T permanent(@NonNull Boolean permanent);


Why is the corresponding method in the MapboxV6BaseGeocoding protected and here it's public?

Builder functions are public because they're supposed to be called by a user. See code example and this comment for more information

Looks like this thread is the same as #1550 (comment), let's continue there.

dzinad · 2023-06-21T08:24:00Z

+  @Override
+  protected Call<V6Response> initializeCall() {
+    final V6RequestOptions requestOptions = requestOptions();
+    if (requestOptions instanceof V6ReverseGeocodingRequestOptions) {


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?

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.

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?

dzinad · 2023-06-21T09:03:55Z

+ * Object containing coordinate parameters (lat, long) and accuracy.
+ */
+@AutoValue
+public abstract class V6Coordinates implements Serializable {


Don't we need a builder for this class?

Mostly the same as here https://github.com/mapbox/mapbox-java/pull/1550/files/eb5084da7fc95edc301986eb8fbb7b9db4a5f3cc#r1237481827

DzmitryFomchyn · 2023-08-21T07:55:56Z

Deferred, not in priority now. Will be reworked once backend in GA.

V6 preview

525276f

DzmitryFomchyn requested a review from a team as a code owner June 20, 2023 13:30

DzmitryFomchyn force-pushed the df-geocoding-v6-preview branch from 1839de2 to 63ab3cf Compare June 20, 2023 13:36

V6 full support

eb5084d

DzmitryFomchyn force-pushed the df-geocoding-v6-preview branch from 63ab3cf to eb5084d Compare June 20, 2023 13:40

dzinad reviewed Jun 21, 2023

View reviewed changes

DzmitryFomchyn force-pushed the df-geocoding-v6-preview branch from c3816bc to 345e23c Compare June 21, 2023 20:51

Code review

255ffa3

DzmitryFomchyn force-pushed the df-geocoding-v6-preview branch from 345e23c to 255ffa3 Compare June 21, 2023 21:21

DzmitryFomchyn closed this Aug 21, 2023

ahmedaly16 mentioned this pull request Apr 16, 2024

Geocoding V6 Support #1577

Merged

Conversation

DzmitryFomchyn commented Jun 20, 2023

Uh oh!

codecov Bot commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DzmitryFomchyn Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzinad Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DzmitryFomchyn Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DzmitryFomchyn commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jun 20, 2023 •

edited

Loading

DzmitryFomchyn Jun 21, 2023 •

edited

Loading

dzinad Jun 22, 2023 •

edited

Loading

DzmitryFomchyn Jun 21, 2023 •

edited

Loading