-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add routable points option and parse results #145
Conversation
MapboxGeocoder/MBPlacemark.swift
Outdated
|
||
if let points = try? container.nestedContainer(keyedBy: RoutableLocationsKeys.self, forKey: .routableLocations), | ||
let coordinatePairs = try points.decodeIfPresent([[CLLocationDegrees]].self, forKey: .points) { | ||
let coordinates = coordinatePairs.map { CLLocationCoordinate2D(geoJSON: $0) } |
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.
Adding support for Codable CLLocationCoordinate2D by adding an extension would be cleaner but wouldn't be worth it until the decoding bottleneck is fixed.
MapboxGeocoder/MBPlacemark.swift
Outdated
/** | ||
An array of locations representing the location a user should navigate to to reach the `Placemark`. | ||
*/ | ||
@objc open var routableLocations: [CLLocationCoordinate2D]? |
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.
routableCoordinates
since it's a list of coordinates?
or, make it [CLLocation]?
so that it also bridges to Objective-C?
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.
Yes, let’s use an array of CLLocation or Placemark, for Objective-C compatibility.
MapboxGeocoder/MBPlacemark.swift
Outdated
/** | ||
An array of locations representing the location a user should navigate to to reach the `Placemark`. | ||
*/ | ||
@objc open var routableLocations: [CLLocationCoordinate2D]? |
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.
Should this property be available on all Placemark objects, or only GeocodedPlacemark objects? Consider that a “routable location” makes little sense for a QualifyingPlacemark.
@@ -64,6 +64,14 @@ open class GeocodeOptions: NSObject { | |||
*/ | |||
@objc open var locale: Locale? | |||
|
|||
|
|||
/** | |||
If true, the response will possibly include a `routableLocation`. |
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.
A Boolean value that determines whether the resulting placemarks have the
Placemark.routableLocation
property set.
/** | ||
If true, the response will possibly include a `routableLocation`. | ||
|
||
`routableLocation` represents the best location for a vehicle to navigate to. |
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.
Let’s remove this line in favor of more documentation for routableLocation
itself.
|
||
`routableLocation` represents the best location for a vehicle to navigate to. | ||
*/ | ||
@objc open var includeRoutableLocations: Bool = false |
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.
Rename this property to includesRoutableLocations
.
MapboxGeocoder/MBPlacemark.swift
Outdated
@@ -352,6 +364,11 @@ open class Placemark: NSObject, Codable { | |||
} | |||
return String(describing: houseNumber) | |||
} | |||
|
|||
/** | |||
An array of locations representing the location a user should navigate to to reach the `Placemark`. |
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.
An array of locations that serve as hints for navigating to the placemark.
If the
GeocodeOptions.includesRoutableLocations
property is set totrue
, this property contains locations that are suitable to use as a waypoint in a routing engine such as MapboxDirections.swift. Otherwise, if theGeocodeOptions.includesRoutableLocations
property is set tofalse
, this property is set tonil
.For the placemark’s geographic center, use the
location
property. The routable locations may differ from the geographic center. For example, if a house’s driveway leads to a street other than the nearest street (by straight-line distance), then this property may contain the location where the driveway meets the street. A route to the placemark’s geographic center may be impassable, but a route to the routable location would end on the correct street with access to the house.
Is anything needed to move this forward, other than bumping CI following the funny business with Travis? |
|
||
For the placemark’s geographic center, use the `location` property. The routable locations may differ from the geographic center. For example, if a house’s driveway leads to a street other than the nearest street (by straight-line distance), then this property may contain the location where the driveway meets the street. A route to the placemark’s geographic center may be impassable, but a route to the routable location would end on the correct street with access to the house. | ||
*/ | ||
@objc open var routableLocations: [CLLocation]? |
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.
I think we should move this property to GeocodedPlacemark, since it doesn’t make sense as a property of QualifyingPlacemark. We’ll need to define an initializer explicitly for GeocodedPlacemark.
/ref #145 (comment)
|
Closes: #144
Adds the option to get routable points back in the response and also an option to request routable points.
Currently, the response will look like:
Paging codable experts @JThramer @frederoni, I'm slightly unsure if this is the preferred method for parsing optional + nested dictionaries.