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

Add Options to JSON Encoder in RouteResponse #437

Closed
wants to merge 0 commits into from

Conversation

hactar
Copy link
Contributor

@hactar hactar commented Jun 20, 2020

When initialising a RouteResponse, the decoder receives the .options passed in. The temporary encoder to encode the routes however does not. This can create a mismatched RouteResponse object, for example if the options state that polyline6 was used.

This PR also passes the .options to the temporary encoder and so fixes an issue for us where the routes are off by 1 decimal position when using polyline6.

@Udumft
Copy link
Contributor

Udumft commented Jul 30, 2020

Hello, @hactar! Thank you for contribution!
I was trying to reproduce the issue you described, but in my attempts RouteResponse object was always decoded correctly, whether I used .polyline6 or usual .polyline format. Could you please provide some more details or a snipped to demonstrate it?

@1ec5
Copy link
Contributor

1ec5 commented Jul 31, 2020

Sorry @hactar, I screwed up and force-pushed this repository’s master to your master branch, blowing away hactar/mapbox-directions-swift@297c00c. I had intended to push an additional commit to your branch, but it was named master, which meant I had to stage it on a branch with a different name on my end. I forgot to pass in my local branch name in addition to the remote branch name. The force-push caused GitHub to automatically close this PR, preventing me from fixing the issue by force-pushing your original commit. I’ve opened #440 with your contribution. Please feel free to force-push your v0.31.1 tag back to your master branch. Also, it’s a good idea to PR branches other than master in the future, to avoid mishaps like this, especially since GitHub allows upstream maintainers to commit directly to PR branches by default. Thanks, and sorry for the inconvenience!

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.

3 participants