-
Notifications
You must be signed in to change notification settings - Fork 92
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
Rename VisualInstruction.degrees
to VisualInstruction.finalHeading
#266
Conversation
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.
Looks good. A couple documentation nits and a question about backwards compatibility.
Note that this property is irrelevant unless the `maneuverType` is | ||
The heading at which you will be exiting a roundabout, assuming 180 indicates going straight through the roundabout. | ||
|
||
Note that this property is irrelevant unless the `maneuverType` is any of |
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.
Double negative: “only relevant if”.
The heading at which you will be exiting a roundabout, assuming 180 indicates going straight through the roundabout. | ||
|
||
Note that this property is irrelevant unless the `maneuverType` is any of | ||
[.takeRoundabout, .takeRotary, .turnAtRoundabout, .exitRoundabout, .exitRotary] |
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.
Surround the cases in backticks and qualify them with ManeuverType.
, so that when we eventually turn on the documentation comment, jazzy will automatically link them.
} | ||
|
||
public func encode(with coder: NSCoder) { | ||
coder.encode(text, forKey: "text") | ||
coder.encode(maneuverType, forKey: "maneuverType") | ||
coder.encode(maneuverDirection, forKey: "maneuverDirection") | ||
coder.encode(degrees, forKey: "degrees") | ||
coder.encode(finalHeading, forKey: "degrees") |
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.
Do you think we should change the key as well, since degrees
has been relatively short-lived?
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.
On the api-directions side, this field is still called "degrees", so when we eventually move to Codable, it would make sense to be able to round-trip api-directions→swift→api-directions, that's why I kept "degrees" in here.
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.
OK. What does this mean for all the other keys that already match the renamed properties?
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.
It would just mean a bit more re-mapping but I'm hoping the contract would be more strongly maintained by a protobuf definition before this becomes relevant.
07925e4
to
4a217cb
Compare
Fixes #265 Rename degrees → finalHeading
@1ec5 👀