Skip to content
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

Fix VisualInstruction.component.lane type mismatch #532

Merged
merged 2 commits into from
Apr 7, 2021

Conversation

jill-cardamon
Copy link
Contributor

This PR fixes a type mismatch in the encoding/decoding of the VisualInstruction.Component.lane.preferredDirection enumeration case as well as the relevant tests.

@jill-cardamon jill-cardamon self-assigned this Apr 5, 2021
@@ -197,7 +197,7 @@ extension VisualInstruction.Component: Codable {
case imageURL
case directions
case isActive = "active"
case activeDirection = "active_direction"
case active_direction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change strictly necessary? activeDirection would make for a better case in Swift; I’d think the raw value would remain unaffected.

@@ -217,7 +217,10 @@ extension VisualInstruction.Component: Codable {
if kind == .lane {
let indications = try container.decode(LaneIndication.self, forKey: .directions)
let isUsable = try container.decode(Bool.self, forKey: .isActive)
let preferredDirection = try container.decodeIfPresent(LaneIndication.self, forKey: .activeDirection)
var preferredDirection: LaneIndication? = nil
if let preferredDirectionDescription = try container.decodeIfPresent(String.self, forKey: .active_direction) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is the same workaround as in #529 (comment) but we needed to apply it to this other field too.

@@ -69,7 +69,7 @@ class VisualInstructionComponentTests: XCTestCase {
"type": "lane",
"active": true,
"directions": ["right", "straight"],
"active_direction": ["right"],
"active_direction": "right",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of error that we often catch through the integration tests that begin with raw JSON fixtures. It’s unfortunate that those fixtures are difficult to maintain though. For now, it’s a reminder to us to double-check these Swift-formatted fixtures against the actual responses coming out of the Directions API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I wonder if there's a reliable way to use an actual response and just parse for what needs to be tested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible, but we’ve chosen not to do that so far, because a dependency on a live network connection can cause a CI job to be unreliable. It would be more appropriate for a CI job that runs on a nightly schedule instead of on every commit or push.

@1ec5 1ec5 added this to the v2.0.0 (RC) milestone Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants