Added walking options #991

devotaaabel · 2019-04-10T15:12:53Z

This is currently blocked until testing on staging is complete.

Guardiola31337 · 2019-04-10T19:54:58Z

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

    @Field("waypoint_targets") String waypointTargets,
-    @Field("enable_refresh") Boolean enableRefresh
+    @Field("enable_refresh") Boolean enableRefresh,
+    @Query("walking_speed") Double walkingSpeed,


Noticed that we're using @Query here instead of @Field and wondering if there are any implications 🤔

@Guardiola31337 My understanding is that @Field is only used for POST because it's @FormUrlEncoded, but if you think this shouldn't be the case let me know

@Guardiola31337 nevermind I just understood what you were saying. It's interesting because I thought they were supposed to be @Field but the example test passed with @Query... I'll update them anyway

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

Guardiola31337 · 2019-04-10T20:05:58Z

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

+      if (walkingOptions() == null) {
+        walkingOptions(WalkingOptions.builder().build());
+      } else if (!DirectionsCriteria.PROFILE_WALKING.equals(profile())) {
+        throw new ServicesException("Walking options are for use with the walking profile");


Does the backend service return any error in this situation? If so, we should bubble up that error and not thrown an exception at this point.

Guardiola31337 · 2019-04-10T20:35:11Z

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

+ * @since 4.6.0
+ */
+@AutoValue
+public abstract class WalkingOptions {


Should we include these options as part of RouteOptions? If so, we should make sure that toJson / fromJson methods are implemented and serialization / deserializations work 👌

osana · 2019-04-11T20:33:37Z

services-core/gradle.properties

@@ -1,4 +1,4 @@
-VERSION_NAME=4.5.0-SNAPSHOT
+VERSION_NAME=4.6.0-SNAPSHOT


I noticed this when I was just releasing 4.6.0.
If you rebase this PR to master - you will not need this commit.

kevinkreiser · 2019-05-09T17:55:01Z

services-directions/src/test/java/com/mapbox/api/directions/v5/MapboxDirectionsTest.java

+      .profile(DirectionsCriteria.PROFILE_WALKING)
+      .walkingOptions(WalkingOptions.builder().alleyBias(2d).build())
+      .build();
+    assertTrue(directions.cloneCall().request().url().toString().contains("alley_bias=2.0"));


is it ok that this test is using a value that is out of bounds? valid values are from -1 to 1

danpaz · 2019-05-09T18:40:55Z

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

+    /**
+     * A factor that modifies the cost when encountering roads or paths that do not allow
+     * vehicles and are set aside for pedestrian use. Pedestrian routes generally attempt to
+     * favor using these walkways and sidewalks. The default walkway_factor is 0.


May want to indicate what the range of bias values mean and their effect. Suggest adding something like: The allowed range of values is from -1 to 1, where -1 indicates indicates preference to _avoid_ pedestrian walkways, 1 indicates preference to _favor_ pedestrian walkways, and 0 indicates no preference. Same comment for alleyBias below.

osana · 2019-05-09T20:03:32Z

@devotaaabel Make sure the @since tags are updated to 4.8.0

Guardiola31337

Overall this looks good @devotaaabel

Left some minor comments to address / discuss before merging here.

Guardiola31337 · 2019-05-09T20:21:16Z

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

  }

  private Call<DirectionsResponse> post() {
+    // todo add walking options


Should we revert these changes back? It seems walking supports POST too now.

Guardiola31337 · 2019-05-09T20:30:17Z

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

+
+  @Nullable
+  Double walkingSpeed() {
+    if (hasWalkingOptions()) {


Minor comment: Guard clauses normally represent the unexpected behavior i.e.

if (!hasWalkingOptions()) { return null; } return walkingOptions().walkingSpeed();

Guardiola31337 · 2019-05-09T21:09:34Z

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

+   * @since 4.8.0
+   */
+  @Nullable
+  public abstract Double walkingSpeed();


Shouldn't options include the @SerializedName? That’s what the backend expects / generates

👀 #991 (comment)

Guardiola31337 · 2019-05-09T21:11:12Z

services-directions/src/test/java/com/mapbox/api/directions/v5/models/WalkingOptionsTest.java

+  import static org.junit.Assert.assertEquals;
+
+public class WalkingOptionsTest extends TestUtils {
+ private static final String JSON = "{\"walkingSpeed\":1.0,\"walkwayBias\":2.0,\"alleyBias\":3.0}";


This JSON isn’t something the backend would return they would return snake_case ~~except for maxHikingDifficulty - wondering if we could ask to services team to make it snake_case too for consistency 🤔~~

I've just 👀 that maxHikingDifficulty is not supported anymore and edited ☝️

osana

This should be good for alpha. Let's give it a spin.

osana · 2019-05-10T13:54:20Z

@devotaaabel
Thank you for working on this PR.
Can I ask you to add some more description on the functionality this PR adds?
I do not think we have an issue open so there is nothing to refer to either.

devotaaabel added Directions Blocked labels Apr 10, 2019

devotaaabel requested review from Guardiola31337 and osana April 10, 2019 15:12

devotaaabel self-assigned this Apr 10, 2019

devotaaabel force-pushed the devota-walking-options branch 2 times, most recently from 3cb42c8 to 6fa2fc8 Compare April 10, 2019 16:36

Guardiola31337 reviewed Apr 10, 2019

View reviewed changes

osana reviewed Apr 11, 2019

View reviewed changes

osana added this to the v4.7.0 milestone Apr 15, 2019

devotaaabel force-pushed the devota-walking-options branch 3 times, most recently from 66ecedc to 5b11c99 Compare April 24, 2019 17:58

osana mentioned this pull request Apr 24, 2019

Release 4.7.0 #1011

Closed

25 tasks

devotaaabel force-pushed the devota-walking-options branch from f70b8d5 to 87cce2d Compare April 25, 2019 19:58

osana removed this from the v4.7.0 milestone May 2, 2019

Devota Aabel added 8 commits May 9, 2019 13:48

Added walking options

b2d1e5e

Updated for feedback

144a4ec

Added toJson and fromJson and added WalkingOptions to RouteOptions

86a1fd3

Made walking options more nullable

5264d5f

Added more tests

0777275

Removed walking options from POST call

2e3f758

Updated per feedback and updated @SInCE annotations

a3a60f7

Updated walking options per updates

b41ad6a

devotaaabel force-pushed the devota-walking-options branch from 2f23f0f to b41ad6a Compare May 9, 2019 17:48

kevinkreiser reviewed May 9, 2019

View reviewed changes

danpaz reviewed May 9, 2019

View reviewed changes

update the javadoc for biasing walkways and alleys

b67b61a

update javadoc once more for the spots i missed

72d3aac

Guardiola31337 reviewed May 9, 2019

View reviewed changes

Devota Aabel added 2 commits May 9, 2019 20:33

Addressed feedback

48a0810

Updated test to reflect accurate values

8016621

osana approved these changes May 10, 2019

View reviewed changes

devotaaabel merged commit 9d98760 into master May 10, 2019

devotaaabel deleted the devota-walking-options branch May 10, 2019 01:15

osana mentioned this pull request May 10, 2019

Release 4.8.0-alpha.2 #1027

Merged

Guardiola31337 mentioned this pull request May 10, 2019

Fix walking options serialization #1028

Merged

		@@ -1,4 +1,4 @@
		VERSION_NAME=4.5.0-SNAPSHOT
		VERSION_NAME=4.6.0-SNAPSHOT

Added walking options #991

Added walking options #991

Uh oh!

Conversation

devotaaabel commented Apr 10, 2019

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!

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!

osana commented May 9, 2019

Uh oh!

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

Guardiola31337 May 9, 2019 • 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!

osana left a comment

Choose a reason for hiding this comment

Uh oh!

osana commented May 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Guardiola31337 May 9, 2019 •

edited

Loading