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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draw slight left/right lanes #1270

Merged
merged 5 commits into from
Apr 4, 2018
Merged

Draw slight left/right lanes #1270

merged 5 commits into from
Apr 4, 2018

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Mar 29, 2018

Fixes #1267 show slight arrows on freeway lanes

Should we also verify the road class? 馃

testslightright_iphone11_2_0x0 3x

@1ec5 @bsudekum 馃憖

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Should we also verify the road class?

It鈥檚 possible for the end of a motorway to have ordinary turn lanes. There are some prominent signalized freeway ends in Washington, D.C., and San Francisco, for example. I think it would be more reliable to treat right as slightRight if the maneuver type is takeOffRamp or reachFork.

@@ -1,6 +1,6 @@
import XCTest
import FBSnapshotTestCase
import MapboxDirections
@testable import MapboxDirections
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting the error: Module 'MapboxDirections' was not compiled for testing

Does something need to change in the cartfile to make this happen?

Copy link
Contributor

@1ec5 1ec5 Apr 2, 2018

Choose a reason for hiding this comment

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

Is this @testable needed? This PR doesn鈥檛 seem to introduce any calls to private methods.

Never mind, it鈥檚 Lane(indications:). Maybe we should make that initializer public. It鈥檚 a good practice to always make at least one initializer public for any class, to facilitate unit testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@frederoni frederoni Apr 3, 2018

Choose a reason for hiding this comment

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

Not sure what the best practice is here but testable imports publicizes private methods in frameworks built with its configuration set to debug (carthage update/bootstrap --configuration Debug) so we can keep the initializer private.

@bsudekum bsudekum added this to the v0.17.0 milestone Apr 2, 2018
@bsudekum bsudekum merged commit 7c46d94 into master Apr 4, 2018
@bsudekum bsudekum deleted the fred/slight-lanes branch April 4, 2018 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants