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

Don't conflate coordinates and waypoints #6507

Closed
wants to merge 1 commit into from
Closed

Conversation

dudeuter
Copy link

Description

Fixes issues with silent waypoints, routes from the EV route planner, and BYOR.

@dudeuter dudeuter added ⚠️ DO NOT MERGE PR should not be merged! needs backporting Requires cherry-picking to a currently running release branch labels Oct 24, 2022
@dudeuter dudeuter requested a review from a team as a code owner October 24, 2022 23:18
@codecov
Copy link

codecov bot commented Oct 24, 2022

Codecov Report

Merging #6507 (429d614) into main (8273101) will not change coverage.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #6507   +/-   ##
=========================================
  Coverage     70.33%   70.33%           
  Complexity     4909     4909           
=========================================
  Files           717      717           
  Lines         27950    27950           
  Branches       3295     3295           
=========================================
  Hits          19659    19659           
  Misses         7013     7013           
  Partials       1278     1278           
Impacted Files Coverage Δ
.../navigation/core/trip/session/MapboxTripSession.kt 86.64% <66.66%> (ø)

@LukasPaczos
Copy link

Tagging @RingerJK as you've been working on #6005 which essentially tackles the same problem (and more).

@RingerJK
Copy link
Contributor

#6005 is ready for review, we should be good to close this one if merge #6005

@kmadsen
Copy link
Contributor

kmadsen commented Nov 1, 2022

@dudeuter will you add tests for this?

@dudeuter
Copy link
Author

dudeuter commented Nov 1, 2022

@dudeuter will you add tests for this?

@RingerJK's work will make this change unnecessary in v2.9+ I was working on this as more of a workaround for this part of the Java SDK not decoding the URL properly.

We might want to update the RouteOptions index URL Path parameters starting from the end. Which might make the decode method a little more robust.

List<String> pathElements = Arrays.asList(url.getPath().split("/"));
Collections.reverse(pathElements);

optionsJson.addProperty("coordinates", URLDecoder.decode(pathElements.get(0), UTF_8));
optionsJson.addProperty("profile", URLDecoder.decode(pathElements.get(1), UTF_8));
optionsJson.addProperty("user", URLDecoder.decode(pathElements.get(2), UTF_8));

That way if anyone's messing around with the basepath they'll be more likely to correctly parse their coordinates, and they can next their API as many arbitrary paths as they want.

@LukasPaczos
Copy link

Since #6005 landed, changes here are not needed anymore.

@dudeuter let's start a separate ticket/PR if you think we should make any changes to how we parse URLs.

@LukasPaczos LukasPaczos closed this Nov 2, 2022
@LukasPaczos LukasPaczos deleted the sd-use-waypoints branch November 2, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ DO NOT MERGE PR should not be merged! needs backporting Requires cherry-picking to a currently running release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants