Expose road classes in route steps #154
Conversation
bsudekum
force-pushed
the
class
branch
2 times, most recently
from
c105841
to
7d1cb0a
Compare
MapboxDirections/MBRoadClass.swift
Outdated
|
|
||
| @objc(MBRoadClass) | ||
| public enum RoadClass: Int, CustomStringConvertible { | ||
| case toll |
There was a problem hiding this comment.
If I understand correctly, the specific classes values coming out of the Directions API are dependent on the profile, correct? If there’s a possibility that a different profile might produce exotic values, we should turn this enum into an extensible string enumeration. (MBDirectionsProfileIdentifier is an example of an extensible string enumeration.) Otherwise, we’d have to document the fact that unrecognized classes could be dropped entirely.
MapboxDirections/MBRoadClass.swift
Outdated
| level = .toll | ||
| case "restricted": | ||
| level = .restricted | ||
| case "highway": |
There was a problem hiding this comment.
Apparently the Directions API will use a motorway class, not a highway class.
There was a problem hiding this comment.
Urg sorry this was my fault. motorway is correct.
| @@ -50,6 +50,8 @@ public class Intersection: NSObject, NSSecureCoding { | |||
| */ | |||
| public let usableApproachLanes: IndexSet? | |||
|
|
|||
| public let roadClasses: [RoadClass] | |||
There was a problem hiding this comment.
An array of enumeration values won’t bridge very cleanly to Objective-C. Additionally, the order of classes doesn’t matter. So we should turn the RoadClass enumeration into a RoadClasses option set (implemented using NS_OPTION in Objective-C), then change this property’s type to simply RoadClasses.
|
@1ec5 this is ready for review (still need to document). |
1ec5
changed the title
| @@ -0,0 +1,10 @@ | |||
| typedef NS_OPTIONS(NSUInteger, MBRoadClasses) { | |||
There was a problem hiding this comment.
All Objective-C files need to import headers for the types they use. This header uses NSUInteger, which is part of Foundation, so import Foundation.h. (This also applies to MBAttribute.h and MBLaneIndication.h.)
MapboxDirections/MBRoadClasses.h
Outdated
| @@ -0,0 +1,10 @@ | |||
| typedef NS_OPTIONS(NSUInteger, MBRoadClasses) { | |||
|
|
|||
| MBRoadClassesToll = (1 << 1), | |||
There was a problem hiding this comment.
MBRoadClasses and its types each need documentation comments.
MapboxDirections/MBRoadClasses.h
Outdated
|
|
||
| MBRoadClassesMotorway = (1 << 3), | ||
|
|
||
| MBRoadClassesFerry = (1 << 4), |
There was a problem hiding this comment.
The documentation comment for this value should discuss its relationship to the ferry transport type, and the documentation for that transport type should likewise refer to this road class.
MapboxDirections/MBRoadClasses.swift
Outdated
|
|
||
| extension RoadClasses: CustomStringConvertible { | ||
|
|
||
| public init?(descriptions: [String]) { |
There was a problem hiding this comment.
This initializer needs a documentation comment as well.
MapboxDirections/MBRoadClasses.swift
Outdated
|
|
||
| public var description: String { | ||
| if isEmpty { | ||
| return "none" |
There was a problem hiding this comment.
It should be possible to round-trip a road class from an array of strings to a RoadClasses and back to a string. Is none a valid string that would appear in a route response? If not, maybe an empty string would be better. Unfortunately, our hands our kind of tied because CustomStringConvertible doesn’t allow for an optional description.
MapboxDirectionsTests/V5Tests.swift
Outdated
| @@ -69,6 +69,8 @@ class V5Tests: XCTestCase { | |||
| let leg = route!.legs.first! | |||
| XCTAssertEqual(leg.name, "I 80, I 80;US 30") | |||
| XCTAssertEqual(leg.steps.count, 59) | |||
| let roadClasses = leg.steps.first!.intersections!.first!.roadClasses! | |||
There was a problem hiding this comment.
Before ! force-unwrapping steps.first and intersections and intersections.first and roadClasses, we should assert that each of these things are non-nil or non-empty. Otherwise we’ll have an unhelpful assertion message when the following assertion fails.
MapboxDirectionsTests/V5Tests.swift
Outdated
| @@ -69,6 +69,8 @@ class V5Tests: XCTestCase { | |||
| let leg = route!.legs.first! | |||
| XCTAssertEqual(leg.name, "I 80, I 80;US 30") | |||
| XCTAssertEqual(leg.steps.count, 59) | |||
| let roadClasses = leg.steps.first!.intersections!.first!.roadClasses! | |||
| XCTAssertTrue(roadClasses.contains([.toll, .restricted])) | |||
There was a problem hiding this comment.
To prevent the test harness from crashing when any of these things is nil, use optionals:
let firstStep = leg.steps.first // as opposed to step below
XCTAssertNotNil(firstStep)
let intersections = firstStep?.intersections
XCTAssertNotNil(intersections)
let intersection = intersections?.first
XCTAssertNotNil(intersection)
let roadClasses = intersection?.roadClasses
XCTAssertNotNil(roadClasses)
XCTAssertTrue(roadClasses?.contains([.toll, .restricted]) ?? false)
The directions API will soon return a class type:
toll,restricted,highwayandferry.This array is found on the intersections key and represents the class type in between each intersection.
The test fixtures are so large right now that working through this task is becoming cumbersome. Going to hold off on this until the API returns the actual keys I'm looking for.
/cc @1ec5 @frederoni