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

fixed an issue where "avoid_maneuver_radius" was represented as integer instead of double #1376

Merged
merged 1 commit into from
Mar 10, 2022

Conversation

LukasPaczos
Copy link
Member

This re-aligns the value with the API. Even though this is technically a breaking change, the integer representation was functionally wrong so we should treat this change as a bug fix.

@LukasPaczos LukasPaczos requested a review from a team March 9, 2022 18:58
@RingerJK
Copy link
Contributor

RingerJK commented Mar 9, 2022

@LukasPaczos I see in docs https://docs.mapbox.com/api/navigation/directions/ number range of avoid_maneuver_radius is [0, 1000]. Are you sure that we need so accurate value, looks like 1 meter should be enough? (Gson is mapping double to integer well)

@LukasPaczos
Copy link
Member Author

The API allows for a number which also accepts floats and doubles, so mapbox-java needs to reflect that. I found this issue out when trying to parse request params created by Nav Native which were a floating point number and led to a crash. So it's less about what makes practical sense and more about the compatibility with the API.

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Breaking SemVer, although we can consider it a bugfix :shipit:

@LukasPaczos LukasPaczos merged commit 6b84207 into main Mar 10, 2022
@LukasPaczos LukasPaczos deleted the lp-maneuver-radius-double branch March 10, 2022 12:24
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