-
Notifications
You must be signed in to change notification settings - Fork 87
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
Mapbox Directions API OSRM Name Fix #222
Conversation
… in the result `json` will produce `""` instead of the expected `nil`. Adding logic to compensate for this.
Would it be possible to add a test case? |
let location = api["location"] as! [Double] | ||
let coordinate = CLLocationCoordinate2D(geoJSON: location) | ||
return Waypoint(coordinate: coordinate, name: local.name ?? api["name"] as? String) | ||
let possibleAPIName = api["name"] as? String | ||
let apiName = (possibleAPIName?.isEmpty ?? true) ? nil : possibleAPIName! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core Navigation extends String with nonEmptyString
, which is pretty convenient for situations like this. Feel free to copy it here, but make it internal, because MapboxDirections.swift shouldn’t be in the business of providing string processing helpers to clients.
fdfe846
to
8c0f938
Compare
I think #217 should be closed as well after this is merged and verified. |
@JThramer mind checking out the test failure:
|
import Foundation | ||
|
||
extension String { | ||
var isNotEmpty: Bool { return !isEmpty } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a bit extreme. For one thing, it’s more verbose than using the !
operator, which is more versatile than hard-coding negative versions of Boolean properties.
This mostly fixes issue mapbox/mapbox-navigation-ios#921, where an empty string is bypassing the
CLGeocoder
and causing the coordinate logic to trigger (which does test for empty string -- a bug, but moot because it's being removed anyway). This change fixes this issue and makes sure thatwaypoint.name == nil
when the Mapbox Directions API does not return an OSRM name for the waypoint.waypoints[n].name
keypath in the resultjson
will produce""
instead of the expectednil
. Adding logic to compensate for this.ToDo:
MBRouteOptions.response(from:)
and tests this edge-caseisNotEmpty
/nonEmptyString
extension and usage/cc @bsudekum @1ec5 @frederoni