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.

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 should be optional #553

Closed
1ec5 opened this issue Jun 17, 2021 · 1 comment · Fixed by #557
Closed

RouteOptions.alleyPriority, walkwayPriority, speed should be optional #553

1ec5 opened this issue Jun 17, 2021 · 1 comment · Fixed by #557
Assignees
Labels
backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Jun 17, 2021

RouteOptions.alleyPriority, walkwayPriority, and speed should be optional, and RouteOptions.urlQueryItems should add the corresponding URL query items if and only if these properties are non-nil.

Problem

RouteOptions only adds the alley_bias parameter to the request URL if the profile is either mapbox/driving or mapbox/walking, but not for other profiles such as mapbox/driving-traffic that also support this parameter.

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

Background

These options are limited to Valhalla-powered profiles and are unsupported in OSRM-based profiles, such as mapbox/cycling and formerly mapbox/driving-traffic. OSRM-based endpoints return an HTTP 422 error upon encountering a parameter that’s supported by Valhalla but not OSRM.

Originally, in #370, this library had to conditionalize the alley_bias parameter on the mapbox/walking profile because it’s supported by Valhalla but not OSRM. #416 later extended the parameter to the mapbox/driving profile once it migrated to Valhalla. We could conditionalize other parameters on the presence or absence of an explicit value, but RouteOptions.alleyPriority had to be non-optional because optional primitive types don’t bridge to Objective-C. Fortunately, #382 dropped Objective-C support; since then, we generally use optional types to represent incremental additions to the Directions and Map Matching APIs, even for primitive types.

Solution

RouteOptions.alleyPriority should become optional, because this library still supports at least one profile (mapbox/cycling) that doesn’t support alley_bias. RouteOptions.urlQueryItems can thus conditionalize the addition of alley_bias on whether RouteOptions.alleyPriority is non-nil.

The walkway_bias and walking_speed parameters added in #370 are in the same situation, so RouteOptions.walkwayPriority and speed should get the same treatment. It would be pretty unlikely for a driving profile to support options specific to walkways, but DirectionsProfileIdentifier is an extensible enumeration and there could always be a walking-related profile other than mapbox/walking.

/cc @mapbox/navigation-ios @danpat @OttyLab

@1ec5 1ec5 added backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work. labels Jun 17, 2021
@1ec5 1ec5 added this to the v2.0.0 (RC) milestone Jun 17, 2021
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 17, 2021

As a workaround, a subclass of RouteOptions can override the urlQueryItems computed property to explicitly append one of these parameters. mapbox/mapbox-navigation-ios-examples#91 gives an example of that in the context of beta parameters, but the same mechanism works for any parameter, even a parameter that this library sometimes appends by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants