Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RouteOptions.alleyPriority, walkwayPriority, speed are now optional #557

Merged
merged 8 commits into from
Jul 8, 2021
Merged

RouteOptions.alleyPriority, walkwayPriority, speed are now optional #557

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fa049dc
initial implementaion
Jun 30, 2021
109e641
update test
Jun 30, 2021
5f0b0a3
remove forced upwrap
Jun 30, 2021
f763838
fixed one broken test
Jun 30, 2021
2e79912
fix other test, remove commented code
Jun 30, 2021
32f8089
update changelog, fix docs, change default usage to medium
Jul 1, 2021
34a3b93
incorporate changelog suggestion
jill-cardamon Jul 7, 2021
695b872
address docs feedback
jill-cardamon Jul 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* Carthage v0.38 or above is now required for installing this SDK if you use Carthage. ([#548](https://github.com/mapbox/mapbox-directions-swift/pull/548))
* Xcode 12.0 or above is now required to build MapboxDirections from source. ([#548](https://github.com/mapbox/mapbox-directions-swift/pull/548))
* You can fully build this SDK on Macs with Apple Silicon. ([#548](https://github.com/mapbox/mapbox-directions-swift/pull/548))
* `RouteOptions.alleyPriority`, `RouteOptions.walkwayPriority`, and `RouteOptions.speed` are now optional, and thus must be set explicitly if the developer wants to include them in the route options. `DirectionsOptions.default` has been renamed to `DirectionsOptions.medium`. ([#557](https://github.com/mapbox/mapbox-directions-swift/pull/557))

## v1.2.0

Expand Down
2 changes: 1 addition & 1 deletion Sources/MapboxDirections/DirectionsOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +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.
*/
static let `default` = DirectionsPriority(rawValue: 0.0)
static let medium = DirectionsPriority(rawValue: 0.0)

/**
The priority level with which a route prefers a particular type of roadway or pathway.
Expand Down
39 changes: 21 additions & 18 deletions Sources/MapboxDirections/RouteOptions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,31 +123,31 @@ open class RouteOptions: DirectionsOptions {
open var roadClassesToAvoid: RoadClasses = []

/**
A number that influences whether the route should prefer or avoid alleys or narrow service roads between buildings.
An optional that represents the number that influences whether the route should prefer or avoid alleys or narrow service roads between buildings.

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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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.
The value of this property must be at least `DirectionsPriority.low` and at most `DirectionsPriority.high`. `DirectionsPriority.medium` neither prefers nor avoids alleys, while a negative value between `DirectionsPriority.low` and `DirectionsPriority.medium` avoids alleys, and a positive value between `DirectionsPriority.medium` and `DirectionsPriority.high` prefers alleys. A value of 0.9 is suitable for pedestrians who are comfortable with walking down alleys.
*/
open var alleyPriority: DirectionsPriority = .default
open var alleyPriority: DirectionsPriority?

/**
A number that influences whether the route should prefer or avoid roads or paths that are set aside for pedestrian-only use (walkways or footpaths).
An optional that represents the number that influences whether the route should prefer or avoid roads or paths that are set aside for pedestrian-only use (walkways or footpaths).

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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 walkways, while a negative value between `DirectionsPriority.low` and `DirectionsPriority.default` avoids walkways, and a positive value between `DirectionsPriority.default` and `DirectionsPriority.high` prefers walkways. A value of −0.1 results in less verbose routes in cities where sidewalks and crosswalks are generally mapped as separate footpaths.
The value of this property must be at least `DirectionsPriority.low` and at most `DirectionsPriority.high`. `DirectionsPriority.medium` neither prefers nor avoids walkways, while a negative value between `DirectionsPriority.low` and `DirectionsPriority.medium` avoids walkways, and a positive value between `DirectionsPriority.medium` and `DirectionsPriority.high` prefers walkways. A value of −0.1 results in less verbose routes in cities where sidewalks and crosswalks are generally mapped as separate footpaths.
*/
open var walkwayPriority: DirectionsPriority = .default
open var walkwayPriority: DirectionsPriority?

/**
The expected uniform travel speed measured in meters per second.
An optional that represents the expected uniform travel speed measured in meters per second.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 value of this property must be at least `CLLocationSpeed.minimumWalking` and at most `CLLocationSpeed.maximumWalking`. The default value is `CLLocationSpeed.normalWalking`.
The value of this property must be at least `CLLocationSpeed.minimumWalking` and at most `CLLocationSpeed.maximumWalking`. `CLLocationSpeed.normalWalking` corresponds to a typical preferred walking speed.
*/
open var speed: LocationSpeed = .normalWalking
open var speed: LocationSpeed?

// MARK: Specifying the Response Format

Expand Down Expand Up @@ -187,12 +187,15 @@ open class RouteOptions: DirectionsOptions {
if includesExitRoundaboutManeuver {
params.append(URLQueryItem(name: "roundabout_exits", value: String(includesExitRoundaboutManeuver)))
}

if profileIdentifier == .automobile || profileIdentifier == .walking {
params.append(URLQueryItem(name: "alley_bias", value: String(alleyPriority.rawValue)))
if let alleyPriority = alleyPriority?.rawValue {
params.append(URLQueryItem(name: "alley_bias", value: String(alleyPriority)))
}

if let walkwayPriority = walkwayPriority?.rawValue {
params.append(URLQueryItem(name: "walkway_bias", value: String(walkwayPriority)))
}
if profileIdentifier == .walking {
params.append(URLQueryItem(name: "walkway_bias", value: String(walkwayPriority.rawValue)))

if let speed = speed {
params.append(URLQueryItem(name: "walking_speed", value: String(speed)))
}

Expand All @@ -219,7 +222,7 @@ open class RouteOptions: DirectionsOptions {

extension LocationSpeed {
/**
By default, pedestrians are assumed to walk at an average rate of 1.42 meters per second (5.11 kilometers per hour or 3.18 miles per hour), corresponding to a typical preferred walking speed.
Pedestrians are assumed to walk at an average rate of 1.42 meters per second (5.11 kilometers per hour or 3.18 miles per hour), corresponding to a typical preferred walking speed.
*/
static let normalWalking: LocationSpeed = 1.42

Expand Down
3 changes: 2 additions & 1 deletion Tests/MapboxDirectionsTests/DirectionsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class DirectionsTests: XCTestCase {
XCTAssertLessThanOrEqual(url.absoluteString.count, MaximumURLLength, "maximumCoordinateCount is too high")

let components = URLComponents(string: url.absoluteString)
XCTAssertEqual(components?.queryItems?.count, 8)
XCTAssertEqual(components?.queryItems?.count, 7)
XCTAssertTrue(components?.path.contains(coordinates.compactMap { $0.requestDescription }.joined(separator: ";")) ?? false)

let request = directions.urlRequest(forCalculating: options)
Expand All @@ -73,6 +73,7 @@ class DirectionsTests: XCTestCase {
func testPOSTRequest() {
let coordinates = Array(repeating: LocationCoordinate2D(latitude: 0, longitude: 0), count: maximumCoordinateCount + 1)
let options = RouteOptions(coordinates: coordinates)
options.alleyPriority = .medium

let directions = Directions(credentials: BogusCredentials)
let request = directions.urlRequest(forCalculating: options)
Expand Down
7 changes: 3 additions & 4 deletions Tests/MapboxDirectionsTests/WalkingOptionsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ class WalkingOptionsTests: XCTestCase {

let options = RouteOptions(waypoints: waypoints, profileIdentifier: DirectionsProfileIdentifier.walking)
var queryItems = options.urlQueryItems

XCTAssertEqual(queryItems.first { $0.name == "alley_bias" }?.value, "0.0")
XCTAssertEqual(queryItems.first { $0.name == "walkway_bias" }?.value, "0.0")
XCTAssertEqual(queryItems.first { $0.name == "walking_speed" }?.value, "1.42")
XCTAssertNil(queryItems.first { $0.name == "alley_bias" }?.value)
XCTAssertNil(queryItems.first { $0.name == "walkway_bias" }?.value)
XCTAssertNil(queryItems.first { $0.name == "walking_speed" }?.value)

options.alleyPriority = DirectionsPriority(rawValue: 0.4)
options.walkwayPriority = DirectionsPriority(rawValue: 0.5)
Expand Down