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

Use Polyline dependency #11

Closed
tmcw opened this issue Apr 8, 2016 · 14 comments
Closed

Use Polyline dependency #11

tmcw opened this issue Apr 8, 2016 · 14 comments
Assignees
Labels

Comments

@tmcw
Copy link

tmcw commented Apr 8, 2016

Just like MapboxDirections.swift, we should depend on the Polyline lib

@1ec5
Copy link
Contributor

1ec5 commented Apr 8, 2016

This will depend on restructuring the project to produce a dynamic framework (#2), much as we've done for MapboxDirections.swift and MapboxGeocoder.swift.

@1ec5
Copy link
Contributor

1ec5 commented May 11, 2016

This will be unblocked once #16 lands.

@tmcw
Copy link
Author

tmcw commented May 12, 2016

#16 landed, can I grab?

@1ec5
Copy link
Contributor

1ec5 commented May 12, 2016

Be my guest!

@tmcw
Copy link
Author

tmcw commented May 12, 2016

👍 waiting on raphaelmor/Polyline#27

@1ec5
Copy link
Contributor

1ec5 commented May 16, 2016

raphaelmor/Polyline#27 is merged and Polyline’s podspec has been bumped to 3.1.0, but CocoaPods’ central repository hasn’t been updated yet. Since the polyline functionality in this repository already works, we should wait until CocoaPods gets updated. Otherwise, downstream users will have to include a second line in their podfile to override the version that CocoaPods resolves the pod to.

@1ec5
Copy link
Contributor

1ec5 commented Jun 6, 2016

Polyline has been updated with support for all the platforms this library supports.

@incanus
Copy link
Contributor

incanus commented Nov 9, 2016

Counterargument: polyline encoding/decoding is so small and lightweight, we might be able to get away with just sourcing this internally. For example here is a port of our JS version that I did recently: https://gist.github.com/incanus/47737ead1b92ff12095070ca11c493d0

@tmcw
Copy link
Author

tmcw commented Nov 10, 2016

I'm happy to vendor an existing Polyline lib, but hesitant to write one ourselves that doesn't have a separate bug tracker. The basic polyline algorithm looks simple, but in mapbox/polyline we've learned about a sneaky class of errors around the particular behavior of number handling in JavaScript - I'd expect similar tiny but significant problems from any new implementation in any language, and want to make sure we have a place to fix them once and for all if/when we find them.

@1ec5
Copy link
Contributor

1ec5 commented Nov 10, 2016

@incanus, are there any substantive differences between your implementation and the one in the Polyline pod?

@incanus
Copy link
Contributor

incanus commented Nov 10, 2016

No, my thinking was about brevity and about less dependencies (even if our own, external dependency, it’s still easier IMHO) but I don’t feel strongly. I would want tests, yes, to exercise what I have written better. You two know better than I if dependencies have been enough of a problem or not.

@1ec5
Copy link
Contributor

1ec5 commented Feb 23, 2017

Now that this project uses Carthage for dependency management and is written in Swift 3, it should be a lot more straightforward for this project to depend on Polyline v4.0 the way MapboxDirections.swift does.

@1ec5 1ec5 self-assigned this Mar 6, 2017
@1ec5
Copy link
Contributor

1ec5 commented Mar 14, 2017

#53 introduces a dependency on Polyline. Of interest, one of the test fixtures had to be changed because of an encoding difference, but the classic Static API output remained unchanged.

@1ec5 1ec5 added the build label Mar 14, 2017
@tmcw tmcw closed this as completed Apr 11, 2017
@1ec5 1ec5 reopened this Apr 11, 2017
@1ec5
Copy link
Contributor

1ec5 commented Apr 11, 2017

#53 needs some conflict resolution, but we’ll close this issue once that PR lands.

@1ec5 1ec5 closed this as completed in #53 Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants