Expose include-hov/hot parameters and add exclude list option #1296

Guardiola31337 · 2021-09-16T22:41:59Z

Fixes Expose include-hov/hot params in RouteOptions #1293
Adds exclude list option Expose include-hov/hot parameters and add exclude list option #1296 (comment)

Opening this as a Draft until Directions team deploys include params from their side in production. For now, we can test it in Staging using a SNAPSHOT from https://github.com/mapbox/mapbox-navigation-android

cc @browndp08 @ktatterso @kelseylivingston @zugaldia @jill-cardamon

Guardiola31337 · 2021-09-16T22:42:25Z

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

  public static final String EXCLUDE_RESTRICTED = "restricted";

+  /**
+   * TBD.


@ktatterso could you share the docs?

Guardiola31337 · 2021-09-16T22:42:30Z

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

+  public static final String INCLUDE_HOV2 = "hov2";
+
+  /**
+   * TBD.


@ktatterso could you share the docs?

Guardiola31337 · 2021-09-16T22:44:23Z

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

+   * Include certain road types from routing. The default is to not include anything from the
+   * profile selected. The following include flags are available for each profile:


@ktatterso is this accurate? If not, could you share the docs?

Suggested change

* Include certain road types from routing. The default is to not include anything from the

* profile selected. The following include flags are available for each profile:

* Include certain road types in routing. By default, none of the road types listed below are included. The following include flags are available for each profile:

Guardiola31337 · 2021-09-16T22:44:36Z

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

+  public static final String INCLUDE_HOV3 = "hov3";
+
+  /**
+   * TBD.


@ktatterso could you share the docs?

zugaldia · 2021-09-23T13:52:02Z

API support for this feature has landed, we're good to proceed merging this PR and issuing a release.

/cc: @browndp08

LukasPaczos · 2021-09-23T14:02:51Z

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

+   * Include certain road types from routing. The default is to not include anything from the
+   * profile selected. The following include flags are available for each profile:


Suggested change

* Include certain road types from routing. The default is to not include anything from the

* profile selected. The following include flags are available for each profile:

* Include certain road types in routing. By default, none of the road types listed below are included. The following include flags are available for each profile:

LukasPaczos · 2021-09-23T14:03:04Z

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

+     * Include certain road types from routing. The default is to not include anything from the
+     * profile selected. The following include flags are available for each profile:


Same as above.

LukasPaczos · 2021-09-23T14:04:55Z

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

+     * @return this builder for chaining options together
+     */
+    @NonNull
+    public abstract Builder include(@Nullable @DirectionsCriteria.IncludeCriteria String include);


Can you include only one parameter, or can it be a list? If the latter, we should also expose a List method variant and test it.

Similar to exclude https://github.com/mapbox/mapbox-navigation-android/blob/d1218a77e305e04672754835df5fe22a4a68985a/libnavigation-base/src/main/java/com/mapbox/navigation/base/route/RouteExclusions.kt#L15-L21 should we do implement that for include from the SDK side for consistency? 🤔

Hmm, this should be part of the mapbox-java for both include and exclude, same as all the other list paramaters.

Guardiola31337 · 2021-09-23T17:42:58Z

Addressed the feedback. This is ready for another round of eyes @mapbox/navigation-android

abhishek1508

Could we add some more tests on what happens if we exclude or include some criteria for a profile that is not supported? For ex: a test for what happens if Profile.Walking and you add Include.HOV2

abhishek1508 · 2021-09-23T17:50:47Z

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

-      .exclude(DirectionsCriteria.EXCLUDE_TOLL)
+      .excludeList(new ArrayList<String>() {{
+          add(DirectionsCriteria.EXCLUDE_TOLL);
+          add(DirectionsCriteria.EXCLUDE_TUNNEL);


This test uses a DirectionsCriteria.PROFILE_DRIVING_TRAFFIC and excludes TUNNEL, but TUNNEL exclude is not mentioned in the documentation above. If DRIVING and DRIVING_TRAFFIC both support TUNNEL as exclude could you please add it to the documentation

That is correct. That being said, Directions team is working on supporting the DRIVING_TRAFFIC profile in the short term mapbox/mapbox-navigation-android#3844 (comment)

In order to be more accurate, I can fix the tests to use DRIVING profile instead 👍

Addressed this.

Thanks for addressing the feedback, but I meant this:

A list of exclude. Exclude certain road types from routing. * The default is to not exclude anything from the profile selected. * The following exclude flags are available for each profile: * <p> * {@link DirectionsCriteria#PROFILE_DRIVING}: One of {@link DirectionsCriteria#EXCLUDE_TOLL}, * {@link DirectionsCriteria#EXCLUDE_MOTORWAY}, or {@link DirectionsCriteria#EXCLUDE_FERRY}.

In the test you are using EXCLUDE_TUNNEL for PROFILE_DRIVING, but the documentation does not mention that it is supported. Could you add {@link DirectionsCriteria#EXCLUDE_TUNNEL} to the documenation?

Going to fix the tests and use ferry for example. tunnel is not supported based on https://docs.mapbox.com/api/navigation/directions/

Profile Available excludes

mapbox/driving One or more of toll, motorway, ferry, or unpaved

mapbox/driving-traffic One or more of toll, motorway, ferry, or unpaved

mapbox/walking No excludes supported

mapbox/cycling ferry

Thanks for confirming @danpat

Guardiola31337 · 2021-09-23T18:34:50Z

Could we add some more tests on what happens if we exclude or include some criteria for a profile that is not supported?

I think those are developer misconfigurations that we shouldn't handle from the client side. We threw exceptions in the past but we decided it wasn't a good idea and let Directions API handle it. Actually if tomorrow Directions API wants to support one more profile we'd need to change it from our side and cut a new release. I'd vote for avoiding that.

Guardiola31337 · 2021-09-23T18:49:11Z

Thanks for the feedback @abhishek1508 I've addressed it so this is ready again for another round of 👀

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

Guardiola31337 · 2021-09-23T20:01:55Z

@kmadsen @abhishek1508 OK ready for a final round of 👀

abhishek1508

Thanks for addressing the feedback @Guardiola31337

kmadsen · 2021-09-23T20:06:55Z

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

  public abstract String exclude();

+  /**
+   * A list of exclude.


Don't think this is needed. Redundant with the @return and there is already a description

Suggested change

* A list of exclude.

kmadsen · 2021-09-23T20:07:18Z

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

+  public abstract String include();
+
+  /**
+   * A list of include.


Same as above, don't think this is needed.

Suggested change

* A list of include.

kmadsen · 2021-09-23T20:07:44Z

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

    public abstract Builder exclude(@Nullable @DirectionsCriteria.ExcludeCriteria String exclude);

+    /**
+     * A list of exclude. Exclude certain road types from routing.


Suggested change

* A list of exclude. Exclude certain road types from routing.

* Exclude certain road types from routing.

Guardiola31337 added ⚠️ DO NOT MERGE Ready For Review Directions labels Sep 16, 2021

Guardiola31337 added this to the 6.0.0 milestone Sep 16, 2021

Guardiola31337 requested a review from a team September 16, 2021 22:41

Guardiola31337 self-assigned this Sep 16, 2021

Guardiola31337 commented Sep 16, 2021

View reviewed changes

Guardiola31337 force-pushed the pg-add-include branch from 8801bfb to 1e30d13 Compare September 20, 2021 19:19

LukasPaczos reviewed Sep 23, 2021

View reviewed changes

Guardiola31337 force-pushed the pg-add-include branch from 1e30d13 to 9695ba6 Compare September 23, 2021 17:40

Guardiola31337 changed the title ~~Expose include-hov/hot parameters~~ Expose include-hov/hot parameters and add exclude list option Sep 23, 2021

abhishek1508 reviewed Sep 23, 2021

View reviewed changes

Guardiola31337 force-pushed the pg-add-include branch from 9695ba6 to a57b1c4 Compare September 23, 2021 18:44

kmadsen reviewed Sep 23, 2021

View reviewed changes

services-directions/src/main/java/com/mapbox/api/directions/v5/DirectionsService.java Show resolved Hide resolved

Guardiola31337 removed the ⚠️ DO NOT MERGE label Sep 23, 2021

expose include-hov/hot parameters and add exclude list option

8eb90a9

Guardiola31337 force-pushed the pg-add-include branch from a57b1c4 to 8eb90a9 Compare September 23, 2021 20:01

abhishek1508 approved these changes Sep 23, 2021

View reviewed changes

Guardiola31337 merged commit 42fa584 into main Sep 23, 2021

Guardiola31337 deleted the pg-add-include branch September 23, 2021 20:06

kmadsen reviewed Sep 23, 2021

View reviewed changes

Guardiola31337 mentioned this pull request Sep 23, 2021

Remove unnecessary comment from Javadocs #1298

Merged

		* Include certain road types from routing. The default is to not include anything from the
		* profile selected. The following include flags are available for each profile:

	* Include certain road types from routing. The default is to not include anything from the
	* profile selected. The following include flags are available for each profile:
	* Include certain road types in routing. By default, none of the road types listed below are included. The following include flags are available for each profile:

Profile	Available excludes
mapbox/driving	One or more of `toll`, `motorway`, `ferry`, or `unpaved`
mapbox/driving-traffic	One or more of `toll`, `motorway`, `ferry`, or `unpaved`
mapbox/walking	No excludes supported
mapbox/cycling	`ferry`

	* A list of exclude. Exclude certain road types from routing.
	* Exclude certain road types from routing.

Expose include-hov/hot parameters and add exclude list option #1296

Expose include-hov/hot parameters and add exclude list option #1296

Uh oh!

Conversation

Guardiola31337 commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

zugaldia commented Sep 23, 2021

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!

Guardiola31337 commented Sep 23, 2021

Uh oh!

abhishek1508 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!

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!

Guardiola31337 commented Sep 23, 2021

Uh oh!

Guardiola31337 commented Sep 23, 2021

Uh oh!

Uh oh!

Guardiola31337 commented Sep 23, 2021

Uh oh!

abhishek1508 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Guardiola31337 commented Sep 16, 2021 •

edited

Loading