RouteOptions and MapboxDirections refactoring #1118

korshaknn · 2020-01-30T13:09:43Z

Curent PR also has changes from #1115

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

korshaknn · 2020-01-30T14:25:59Z

Also, maybe I should make all list params in MapboxDirections Nullable? to make it possible to remove remove unnecessary ones?

LukasPaczos

Could you drop changes from #1115? We don't actually want to merge them into master.

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java

LukasPaczos · 2020-01-30T17:55:57Z

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java

-        approaches[i] = "";
-      } else if (!approaches[i].equals("unrestricted")
-        && !approaches[i].equals("curb") && !approaches[i].isEmpty()) {
+  public static String formatApproaches(List<String> approaches) {


Same as above.

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

Guardiola31337

Left a couple of minor comments.

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java

Guardiola31337 · 2020-02-05T17:31:58Z

@korshaknn

what about waypointIndices, can we skip them passing an empty string or a null value?

Yeah, as indicated in the docs (👀 at the Note)

   * @param waypointIndices     which input coordinates should be treated as waypoints/separate legs
   *                            Note: coordinate indices not added here act as silent waypoints

we can skip them. But we need to be careful in this particular case because the valid formatting that the Directions API is expecting (imagine we have 3 waypoints) would be waypoints=0;2 (✅) and not waypoints=0;;2 (❌) - note that "" trail is invalid, while it's needed for other parameters. Does that make sense? Let me know if this example isn't clear enough to understand the subtle difference. cc @danpaz

uuid and walkingOptions are Nullable in DirectionsResponse, but NonNull in RouteOptions. Which one should I use?

uuid it's in the DirectionsResponse and it's not always set so it should be @Nullable.

Walking parameters are optional 👀 https://docs.mapbox.com/api/navigation/#optional-parameters-for-the-mapboxwalking-profile so @Nullable as well.

Guardiola31337 · 2020-02-05T17:38:58Z

In any case, in order to verify all these "unknowns" / questions and make sure everything is working as expected, it'd be 💯 if we can create a SNAPSHOT and test downstream from the Navigation SDK side.

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

...es-directions-models/src/test/java/com/mapbox/api/directions/v5/models/RouteOptionsTest.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

LukasPaczos · 2020-02-12T10:25:12Z

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java

+      return null;
+    }
+
+    // trailing nulls should be dropped


Are we sure that this is correct @korshaknn @Guardiola31337 @danpaz?

If I understand correctly, the nulls/empty strings should only be avoided for waypoints. For fields like bearings or radiuses we still want to have all of the nulls/empty strings be represented, even if they are trailing because the number of arguments has to match the number of coordinates.

current code works with Strings and split is used to parse input string.
split method removes trailing empty strings, so I just implemented the same logic, but with Lists
please check MapboxDirectionsUtils, to parse Strings we use Kotlin
someString.split(SEMICOLON.toRegex()).dropLastWhile { it.isEmpty() }
so, we drop empty strings. (it works exactly as java split does).
@LukasPaczos @Guardiola31337

Is that what Directions API expects though? The number of input values is going to differ in comparison to the size of the coordinate, isn't it?

Yeah @LukasPaczos is right here 👇

If I understand correctly, the nulls/empty strings should only be avoided for waypoints. For fields like bearings or radiuses we still want to have all of the nulls/empty strings be represented, even if they are trailing because the number of arguments has to match the number of coordinates.

We need to make sure the number of coordinates match with the number of coordinates (except for waypointIndices as discussed above).

cc @danpaz

...es-directions-models/src/test/java/com/mapbox/api/directions/v5/models/RouteOptionsTest.java

Guardiola31337

Left some minor comments.

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/ParseUtils.java

LukasPaczos

LGTM. Did you have a chance to verify if snapshot works in the Nav SDK?

LukasPaczos · 2020-02-13T12:42:45Z

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java

+   */
+  @Nullable
+  public static String join(@NonNull CharSequence delimiter, @Nullable List<?> tokens,
+      boolean removeTrailingNulls) {


is removeTrailingNulls always false? Maybe we can remove it if that's the case? One less thing to test and maintain.

it is true for waypointIndices

waypointIndices(FormatUtils.join(";", waypointIndices, true));

Do we need it though? The docs are clear that we are looking for indices and passing nulls will be invalid, and the response error will point it out probably. Removing values under the hood might just introduce unexpected behavior.

I added it just to try to handle customers invalid input String (or list with trailing nulls).
But maybe you are right and we should not do it under the hood.

LukasPaczos · 2020-02-13T12:49:00Z

Awesome work!

Guardiola31337

Great work @korshaknn 💪

Left a minor comment and noticed that we're not adding @NonNull annotation to fromJson 👀

mapbox-java/services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java

Line 507 in d466d1d

public static RouteOptions fromJson(String json) {

Other than that

Guardiola31337 · 2020-02-13T13:29:25Z

circle.yml

          name: Publish Java libraries to Bintray
          command: |
-            if [[ $CIRCLE_BRANCH == master ]] || [[ $CIRCLE_TAG == v* ]]; then
+            if [[ $CIRCLE_BRANCH == knn-options-refactoring ]] || [[ $CIRCLE_TAG == v* ]]; then


Revert this before merging.

This still applies.

korshaknn · 2020-02-14T13:14:22Z

@LukasPaczos @Guardiola31337 during snapshot testing I found some issues with MapboxDirections and annotations inside RouteOptions. Please check my last commit

Also I removed methods from MapboxDirections which had varargs param, now only methods with Lists available.

services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java

...es-directions-models/src/test/java/com/mapbox/api/directions/v5/models/RouteOptionsTest.java

Guardiola31337 · 2020-02-14T18:52:58Z

@korshaknn #1118 (comment)

we can't iterate on them, they are just static constants.
I can remove that check, docs have full description according valid params.

Yeah, before we had @AnnotationCriteria

mapbox-java/services-directions/src/main/java/com/mapbox/api/directions/v5/MapboxDirections.java

Lines 593 to 596 in 6b53e5e

    
           @Deprecated 
        
           public Builder annotations(@AnnotationCriteria @NonNull String... annotations) { 
        
             return annotations(Arrays.asList(annotations)); 
        
           }

but now we cannot control that in List<String> unless we create an Annotation type. But yeah, as you mentioned for now it's fine as it's properly documented.

Guardiola31337

Thanks for addressing the feedback @korshaknn

I left some minor comments but not necessarily blocking the PR.

Let's see if these new APIs make sense when used from a client e.g. the Navigation SDK mapbox/mapbox-navigation-android#2477 and iterate if not 🚀

Guardiola31337 · 2020-02-14T18:20:02Z

circle.yml

          name: Publish Java libraries to Bintray
          command: |
-            if [[ $CIRCLE_BRANCH == master ]] || [[ $CIRCLE_TAG == v* ]]; then
+            if [[ $CIRCLE_BRANCH == knn-options-refactoring ]] || [[ $CIRCLE_TAG == v* ]]; then


This still applies.

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java

Guardiola31337 · 2020-02-14T18:51:32Z

...es-directions-models/src/test/java/com/mapbox/api/directions/v5/models/RouteOptionsTest.java

+    annotations.add(ANNOTATION_DISTANCE);
+    annotations.add(ANNOTATION_MAXSPEED);
+    annotations.add(ANNOTATION_SPEED);
+    annotations.add(null);


Not sure if we want to accept null and then trim them 🤔 But it's good for now.

passed Annotations List might have null values. I think we can't forbid it, so we need to have logic to remove them. Also we can update docs (in a separate PR) to make it valid params clearer.

Guardiola31337 · 2020-02-14T18:52:48Z

...es-directions-models/src/test/java/com/mapbox/api/directions/v5/models/RouteOptionsTest.java

+
+  @Test
+  public void annotationsString() {
+    String annotationsStr = ANNOTATION_MAXSPEED + ";" + ANNOTATION_DURATION + ";" + ";";


Same here. Not sure if we want to accept unnecessary ; and then trim them 🤔 In any case, it's good for now 👍

see the comment above

korshaknn requested review from Guardiola31337, Lebedinsky, LukasPaczos and RingerJK January 30, 2020 13:09

korshaknn self-assigned this Jan 30, 2020

RingerJK reviewed Jan 30, 2020

View reviewed changes

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java Outdated Show resolved Hide resolved

RingerJK reviewed Jan 30, 2020

View reviewed changes

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java Outdated Show resolved Hide resolved

LukasPaczos reviewed Jan 30, 2020

View reviewed changes

Guardiola31337 reviewed Jan 30, 2020

View reviewed changes

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java Outdated Show resolved Hide resolved

services-core/src/main/java/com/mapbox/core/utils/TextUtils.java Outdated Show resolved Hide resolved

korshaknn force-pushed the knn-options-refactoring branch 14 times, most recently from 4beac8c to 3414cde Compare February 3, 2020 07:25

LukasPaczos reviewed Feb 3, 2020

View reviewed changes

korshaknn force-pushed the knn-options-refactoring branch 2 times, most recently from c38d662 to 76de915 Compare February 3, 2020 12:23

RingerJK reviewed Feb 3, 2020

View reviewed changes

services-directions-models/src/main/java/com/mapbox/api/directions/v5/utils/FormatUtils.java Outdated Show resolved Hide resolved

korshaknn force-pushed the knn-options-refactoring branch 2 times, most recently from fe0b214 to 1d4d6e6 Compare February 3, 2020 12:40

Guardiola31337 reviewed Feb 11, 2020

View reviewed changes

LukasPaczos reviewed Feb 12, 2020

View reviewed changes

Guardiola31337 reviewed Feb 12, 2020

View reviewed changes

LukasPaczos approved these changes Feb 13, 2020

View reviewed changes

korshaknn force-pushed the knn-options-refactoring branch from beab146 to b717d11 Compare February 13, 2020 13:02

Guardiola31337 approved these changes Feb 13, 2020

View reviewed changes

korshaknn force-pushed the knn-options-refactoring branch 3 times, most recently from 2256359 to 1ebcd68 Compare February 14, 2020 13:12

Guardiola31337 reviewed Feb 14, 2020

View reviewed changes

korshaknn force-pushed the knn-options-refactoring branch 3 times, most recently from 4d8016c to 6b53e5e Compare February 14, 2020 15:53

Guardiola31337 approved these changes Feb 14, 2020

View reviewed changes

options refactoring, lists are used instead of strings. Tests updated

1b74523

korshaknn force-pushed the knn-options-refactoring branch from 6b53e5e to 1b74523 Compare February 17, 2020 08:16

korshaknn marked this pull request as ready for review February 17, 2020 08:21

korshaknn merged commit 123e7cd into master Feb 17, 2020

LukasPaczos mentioned this pull request Feb 19, 2020

RouteOptions parsing issuse #1120

Closed

This was referenced Feb 21, 2020

Bearing formatting fix #1121

Merged

RouteOptions parsing improvements #1117

Closed

MapboxDirections builder throws Exception when approaches is empty. #1081

Closed

Guardiola31337 mentioned this pull request Mar 1, 2020

RouteOptions IllegalStateException - RouteOptions backwards incompatible #1122

Closed

langsmith mentioned this pull request Mar 23, 2020

Release 5.1.0-beta.2 #1131

Closed

2 tasks

LukasPaczos deleted the knn-options-refactoring branch June 3, 2021 12:07

RouteOptions and MapboxDirections refactoring #1118

RouteOptions and MapboxDirections refactoring #1118

Uh oh!

Conversation

korshaknn commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

korshaknn commented Jan 30, 2020

Uh oh!

LukasPaczos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Guardiola31337 commented Feb 5, 2020

Uh oh!

Guardiola31337 commented Feb 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukasPaczos Feb 12, 2020 • 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!

korshaknn Feb 12, 2020 • 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!

Guardiola31337 Feb 12, 2020 • edited by LukasPaczos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukasPaczos left a comment

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!

LukasPaczos Feb 13, 2020 • 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!

korshaknn Feb 13, 2020 • 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!

LukasPaczos commented Feb 13, 2020

Uh oh!

Guardiola31337 left a comment

korshaknn commented Jan 30, 2020 •

edited

Loading

LukasPaczos Feb 12, 2020 •

edited

Loading

korshaknn Feb 12, 2020 •

edited

Loading

Guardiola31337 Feb 12, 2020 •

edited by LukasPaczos

Loading

LukasPaczos Feb 13, 2020 •

edited

Loading

korshaknn Feb 13, 2020 •

edited

Loading

korshaknn commented Feb 14, 2020 •

edited

Loading