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.

Sign up for GitHub

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

Fix route v8 multiple via points #76

Merged
merged 6 commits into from
Mar 31, 2024
Merged

Fix route v8 multiple via points #76

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ea7aa2
Fix route v8 multiple via points
Mar 28, 2024
271d160
Clean test
Mar 28, 2024
e991034
Merge pull request #1 from cecileprat/fix_multiple_viapoints
cecileprat Mar 28, 2024
fb3dc55
Bumping codecov version
Mar 29, 2024
9a75331
Bump responses version
Mar 29, 2024
5ac00a6
Merge branch 'master' into master
abdullahselek Mar 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion herepy/routing_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ def route_v8(
via_keys = []
if via:
for i, v in enumerate(via):
key = str.format("{0}{1}", "via", i)
key = str.format("{0}{1}_", "via", i)
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of using _ here?

Copy link
Author

@cecileprat cecileprat Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you have 11 via points, you will have via1 and via10 in data and manipulation_keys

The first url built in __get will contain: via1=...&via10=..

Then we loop to replace those manipulation_keys (via1 and via10) by the one needed by HERE (via):

  • url.replace("via1", "via") -> via=...&via0=... (it also replaces the first part of via10)
  • url.replace("via10", "via") -> via=...&via0=... (it does nothing, so the second via point is not given through the via query parameter, and so HERE does not take the second via point into account)

By creating via1_ and via10_ keys instead:

given the url containing via1_=...&via10_=..

  • url.replace("via1_", "via") -> via=...&via10_=... (it replaces only the first key)
  • url.replace("via10_", "via") -> via=...&via=... (it replaces the second key)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the clarification 👍 changes look good to me.

Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind bumping the version of codecov to make the CI happy?

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for bumping the codecov version, it seems like your test is failing. Let's fix it and then we can merge.

Copy link
Owner

Choose a reason for hiding this comment

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

Responses requires Python 3.8 or higher as I see, can you remove the previous versions from CI config?

Copy link
Author

@cecileprat cecileprat Mar 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do you want me to remove it from ci.yml and .travis.yml, but keep python_requires=">=3.5" in setup.py?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes please, 3.5 has already reached end of life. It's a good time to stop supporting it. Please update the setup.py too.

via_keys.append(key)
data[key] = str.format("{0},{1}", v[0], v[1])
if departure_time:
Expand Down
22 changes: 22 additions & 0 deletions tests/test_routing_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,3 +1179,25 @@ def test_route_v8_error_access_denied(self):
truck={"shippedHazardousGoods": ["explosive", "gas"]},
scooter={"allowHighway": "true"},
)

@responses.activate
def test_route_v8_multiple_via_points(self):
responses.add(
responses.GET,
"https://router.hereapi.com/v8/routes",
"{}",
status=200,
match=[
responses.matchers.query_param_matcher(
{"via": ["41.9339,-87.9021"] * 11}, strict_match=False
)
],
)
response = self._api.route_v8(
transport_mode=herepy.RoutingTransportMode.car,
origin=[41.9798, -87.8801],
destination=[41.9043, -87.9216],
via=[[41.9339, -87.9021]] * 11,
)
self.assertTrue(response)
self.assertIsInstance(response, herepy.RoutingResponseV8)
Loading