Route Alerts Refinement #506
Conversation
Udumft
added
the
op-ex
Udumft
requested review from
avi-c,
1ec5,
MaximAlien,
dersim-davaod and
chezzdev
There was a problem hiding this comment.
If you have time, it would be great to include a basic encoding/decoding unit test to ensure that the Incident type and its dependent types all round-trip in the usual case. I realize the tests were missing from #466, but any (justified) departure from the JSON format increases the risk of a roundtripping issue.
| /// A list of lane indices, affected by the incident | ||
| public var lanesBlocked: [Int] |
There was a problem hiding this comment.
Are these indices consistent with the indices in Intersection.approachLanes? If so, let’s mention it in this documentation comment.
There was a problem hiding this comment.
In the end I decided to introduce an enum to ease handling these. It is unlikely that humanity invents new types of lanes, but I still left a place to avoid crashing once naming is updated. Even NN does not trust this list to be static.
Unfortunately this is not compatible with LaneIndication we use in Intersections.
There was a problem hiding this comment.
Or am I overthinking it introducing rawLanesBlocked?
| case Lane1 = "1" | ||
| case Lane2 = "2" | ||
| case Lane3 = "3" | ||
| case Lane4 = "4" | ||
| case Lane5 = "5" | ||
| case Lane6 = "6" | ||
| case Lane7 = "7" | ||
| case Lane8 = "8" | ||
| case Lane9 = "9" | ||
| case Lane10 = "10" |
There was a problem hiding this comment.
For the documentation comments for these cases, let’s say e.g. “Second lane from the left”, assuming that’s what lane2 means even in a region that drives on the left.
There was a problem hiding this comment.
Doesn't "Lane1" means the leftmost lane for Left Driving Side regions and the rightmost lane for Right Driving Side regions?
There was a problem hiding this comment.
I was assuming the numbering follows the California standard, but other regions may number the lanes differently, so maybe we need to be ambiguous for now.
There was a problem hiding this comment.
The National Traffic Information Management Coalition, which is affiliated with AASHTO in the U.S., has been recommending the California lane numbering scheme for use nationwide. Michigan and Ohio have both officially adopted it for TIMs.
| let formatter = ISO8601DateFormatter() | ||
|
|
||
| try container.encode(identifier, forKey: .identifier) | ||
| try container.encode(rawKind, forKey: .type) |
There was a problem hiding this comment.
Do we need a separate rawKind property? I think we can make Kind conform to Codable and it’ll automatically be possible to replace rawKind with kind here.
There was a problem hiding this comment.
rawKind is introduced to preserve JSON encoding/decoding roundtrip for cases when current SDK version does not support new types of Incidents. Thus, we just store the raw value for coding purposes, and at the same time user is enabled to work only with known defined Kinds.
Same approach applied to BlockedLane, but IMHO it is less likely that new types of lanes could be introduced then new types of incidents. So I would rather agree that we should get rid of rawLanesBlocked instead.
There was a problem hiding this comment.
To avoid parallel properties, we should store only the raw value and make the enumeration-typed property a computed property.
| /// Left and center lanes | ||
| case leftCenter = "LEFT CENTER" |
There was a problem hiding this comment.
According to this presentation (slide 11), traffic information management (TIM) systems in the U.S. use “left center lane” to refer to the second lane from left on a four-lane highway:
Note that this slide isn’t globally applicable: for example, inside and outside would be reversed in everyday speech in Taiwan. But I don’t know if that would affect the incident data we get.
| case Lane1 = "1" | ||
| case Lane2 = "2" | ||
| case Lane3 = "3" | ||
| case Lane4 = "4" | ||
| case Lane5 = "5" | ||
| case Lane6 = "6" | ||
| case Lane7 = "7" | ||
| case Lane8 = "8" | ||
| case Lane9 = "9" | ||
| case Lane10 = "10" |
There was a problem hiding this comment.
The National Traffic Information Management Coalition, which is affiliated with AASHTO in the U.S., has been recommending the California lane numbering scheme for use nationwide. Michigan and Ohio have both officially adopted it for TIMs.
…mments for Blocked lane cases. Made computed properties.
…. Unit test updated
| import Foundation | ||
|
|
||
| /// Defines a lane affected by the `Incident` | ||
| public struct BlockedLanes: OptionSet, CustomStringConvertible { |
There was a problem hiding this comment.
Oh cool, thanks for taking care of turning this type into an option set.
| public var kind: Kind? { | ||
| return Kind(rawValue: rawKind) | ||
| } | ||
| var rawKind: String |
There was a problem hiding this comment.
Something for consideration before the final release: now that we’re writing in pure Swift, we could consolidate these two properties into a single kind property of type Kind, but one of the Kind cases could have an associated value with the raw type. This would in fact be an option for any enumeration that the Directions API reserves the right to extend, other than option sets. But until we go down that road, the separate rawKind property is appropriate.
Prepares Route Alerts related types for public usage.
This PR refines Docs for number of entities and properties, modifies
Incidentstruct. For the latter I used info provided by Directions team in slack convo and existing Valhalla schema.Fixes #482.