RouteOptions.alleyPriority, walkwayPriority, speed are now optional #557
Conversation
jill-cardamon
added
backwards incompatible
This comment has been minimized.
This comment has been minimized.
| @@ -129,7 +129,7 @@ open class RouteOptions: DirectionsOptions { | |||
|
|
|||
| The value of this property must be at least `DirectionsPriority.low` and at most `DirectionsPriority.high`. The default value of `DirectionsPriority.default` neither prefers nor avoids alleys, while a negative value between `DirectionsPriority.low` and `DirectionsPriority.default` avoids alleys, and a positive value between `DirectionsPriority.default` and `DirectionsPriority.high` prefers alleys. A value of 0.9 is suitable for pedestrians who are comfortable with walking down alleys. | |||
There was a problem hiding this comment.
The comment needs an update. There is no "default" anymore. Also, it needs an explanation of what 'nil' means.
There was a problem hiding this comment.
Same for the other properties too.
There was a problem hiding this comment.
Thank you both! I will update the docs. Some examples can be updated after the next release. I can open a ticket for tail work.
| @@ -99,6 +99,7 @@ public struct DirectionsPriority: Hashable, RawRepresentable { | |||
| /** | |||
| The priority level with which a route neither avoids nor prefers a particular type of roadway or pathway. | |||
| */ | |||
| // should we remove this default (or change its naming) if we don't set it as such? | |||
There was a problem hiding this comment.
Yes, medium (as in “medium priority”) would be better, since apparently the API reserves the right to use a different default at some point in the future, even if that would be somewhat nonsensical for a relative value.
| @@ -129,7 +129,7 @@ open class RouteOptions: DirectionsOptions { | |||
|
|
|||
| The value of this property must be at least `DirectionsPriority.low` and at most `DirectionsPriority.high`. The default value of `DirectionsPriority.default` neither prefers nor avoids alleys, while a negative value between `DirectionsPriority.low` and `DirectionsPriority.default` avoids alleys, and a positive value between `DirectionsPriority.default` and `DirectionsPriority.high` prefers alleys. A value of 0.9 is suitable for pedestrians who are comfortable with walking down alleys. | |||
There was a problem hiding this comment.
Same for the other properties too.
jill-cardamon
force-pushed
the
jill/alleyPriorityOptional-553
branch
from
78a837d to
32f8089
Compare
|
|
||
| This property has no effect unless the profile identifier is set to `DirectionsProfileIdentifier.walking`. You can adjust this property to account for running or for faster or slower gaits. When the profile identifier is set to another profile identifier, such as `DirectionsProfileIdentifier.driving`, this property is ignored in favor of the expected travel speed on each road along the route. This property may be supported by other routing profiles in the future. | ||
| This property has no effect unless it is explicitly set as it defaults to nil, and works best when the profile identifier is set to `DirectionsProfileIdentifier.walking`. You can adjust this property to account for running or for faster or slower gaits. When the profile identifier is set to another profile identifier, such as `DirectionsProfileIdentifier.driving`, this property is ignored in favor of the expected travel speed on each road along the route. This property may be supported by other routing profiles in the future. |
There was a problem hiding this comment.
We named the property just speed on the client side in case there’s ever something like a cycling_speed, to avoid a proliferation of profile-specific properties on the class, but for now it’s only hooked up to walking_speed.
Although this PR technically allows the property to be set in other profiles, I think we should continue to document it as a walking-only option for now. We don’t want developers to get the impression that it works with any profile and get surprised about an error. If for some reason the API were to support walking_speed for, say, the mapbox/driving-traffic profile, that usage would remain possible but undocumented, which I think is OK.
/cc @danpat
|
|
||
| /** | ||
| The expected uniform travel speed measured in meters per second. | ||
| An optional that represents the expected uniform travel speed measured in meters per second. |
There was a problem hiding this comment.
It isn’t necessary to call out the optionality of the type; that’s already clear to the developer in Swift. It is important to note what happens if the property is unspecified: I think we can simply say for all three properties that the Directions API will choose the most reasonable value.
|
|
||
| This property has no effect unless the profile identifier is set to `DirectionsProfileIdentifier.automobile` or `DirectionsProfileIdentifier.walking`. | ||
| This property has no effect unless it is explicitly set as it defaults to nil, and works best when the profile identifier is set to `DirectionsProfileIdentifier.automobile` or `DirectionsProfileIdentifier.walking`. |
There was a problem hiding this comment.
The Directions API documentation doesn’t explicitly restrict this option to any particular profile, even if it happens to be unsupported in the cycling profile, so I think we can remote this note entirely. It’s probably unnecessary to state that an unset property is set to nil and therefore has no effect, since no default value is present in the property signature.
|
|
||
| This property has no effect unless the profile identifier is set to `DirectionsProfileIdentifier.walking`. You can adjust this property to avoid [sidewalks and crosswalks that are mapped as separate footpaths](https://wiki.openstreetmap.org/wiki/Sidewalks#Sidewalk_as_separate_way), which may be more granular than needed for some forms of pedestrian navigation. | ||
| This property has no effect unless it is explicitly set as it defaults to nil, and works best when the profile identifier is set to `DirectionsProfileIdentifier.walking`. You can adjust this property to avoid [sidewalks and crosswalks that are mapped as separate footpaths](https://wiki.openstreetmap.org/wiki/Sidewalks#Sidewalk_as_separate_way), which may be more granular than needed for some forms of pedestrian navigation. |
There was a problem hiding this comment.
The Directions API documentation explicitly limits this option to the mapbox/walking profile. I think it’s safer to retain the documentation as it was, for consistency with the API documentation, even as we remove the formal client-side check.
Co-authored-by: Minh Nguyễn <[email protected]>
This PR addresses #553 by making
RouteOptions.alleyPriority,walkwayPriority, andspeedoptional properties and conditionally adding their corresponding URL query items toRouteOptions.urlQueryItems.