Preferred lane direction is now a ManeuverDirection #535
Conversation
|
Tests are failing because of a nil value error. |
1ec5
added
the
op-ex
| // lanes![i].validIndication = usableLaneIndication | ||
| // } | ||
| // let usable = LaneIndication(descriptions: [usableLaneIndication.rawValue]) | ||
| if lanes![i].indications.contains(LaneIndication(descriptions: [usableLaneIndication.rawValue])!) { |
There was a problem hiding this comment.
This is implicitly reinterpreting a maneuver direction description string as a lane indication description string. That is what we want, but we should make this step more explicit. Let’s create a convenience initializer on LaneIndication that takes a ManeuverDirection. It can switch over all the anticipated description strings to create the corresponding LaneIndication, or it can use the same rawValue-based approach here, but at least it would be better documented that way.
There was a problem hiding this comment.
Another way is to compare the string descriptions of the LaneIndication to the ManeuverDirection's raw value:
lanes![i].indications.descriptions.contains(usableLaneIndication.rawValue).If this comparison is used, is the convenience initializer even necessary?
There was a problem hiding this comment.
I think it would be preferable to make the conversion explicit in some fashion. Otherwise it would be more difficult to find this raw value–based conversion if the assumption ever breaks in the future.
| /** | ||
| The maneuver has no direction. | ||
| */ | ||
| case none |
There was a problem hiding this comment.
Adding this case was necessary for getting "road-tripping" to work properly since there is no initializer for ManeuverDirection that handles none.
There was a problem hiding this comment.
Let’s make the usableLaneIndication property optional instead, since Optional already has a none case.
| // usableLaneIndication = usableIndications.reduce(ManeuverDirection(rawValue: "none")) { $0.union($1) } | ||
| if Set(lanes.compactMap { $0.validIndication}).count > 1 { | ||
| let context = EncodingError.Context(codingPath: decoder.codingPath, debugDescription: "Inconsistent valid indications.") | ||
| throw EncodingError.invalidValue(Set(lanes.compactMap { $0.validIndication}).count, context) |
There was a problem hiding this comment.
Returning the set itself would be more informative than the count of unique indications.
| /** | ||
| The maneuver has no direction. | ||
| */ | ||
| case none |
There was a problem hiding this comment.
Let’s make the usableLaneIndication property optional instead, since Optional already has a none case.
|
| let context = EncodingError.Context(codingPath: decoder.codingPath, debugDescription: "Inconsistent valid indications.") | ||
| throw EncodingError.invalidValue(validIndications, context) | ||
| } | ||
| usableLaneIndication = lanes.compactMap { $0.validIndication }.first |
There was a problem hiding this comment.
| usableLaneIndication = lanes.compactMap { $0.validIndication }.first | |
| usableLaneIndication = validIndications.first |
| @@ -69,6 +69,10 @@ public struct LaneIndication: OptionSet, CustomStringConvertible { | |||
| self.init(rawValue: laneIndication.rawValue) | |||
| } | |||
|
|
|||
| public init?(from direction: ManeuverDirection) { | |||
| self.init(descriptions: [direction.rawValue]) | |||
There was a problem hiding this comment.
Add a comment noting the assumption that every possible raw value of ManeuverDirection matches a valid raw value of LaneIndication.
| @@ -69,6 +69,10 @@ public struct LaneIndication: OptionSet, CustomStringConvertible { | |||
| self.init(rawValue: laneIndication.rawValue) | |||
| } | |||
|
|
|||
| public init?(from direction: ManeuverDirection) { | |||
There was a problem hiding this comment.
Does this method need to be public? If so, add a documentation comment for it.
| // var preferredDirection: ManeuverDirection? = nil | ||
| // if let preferredDirectionDescription = try container.decodeIfPresent(ManeuverDirection.self, forKey: .activeDirection) { | ||
| // preferredDirection = ManeuverDirection(rawValue: preferredDirectionDescription) | ||
| // } |
There was a problem hiding this comment.
Remove this commented-out code.
| @@ -260,15 +260,16 @@ extension Intersection: Codable { | |||
| try container.encode(outletArray, forKey: .outletIndexes) | |||
|
|
|||
| var lanes: [Lane]? | |||
| print("lanes: \(String(describing: lanes))") | |||
There was a problem hiding this comment.
@jill-cardamon This print should be removed
Description
This PR is to change the type of preferred lane direction from
LaneIndicationtoManeuverDirectionto remove the hacks introduced in #529 and #532 and avoid converting within the Nav SDK.