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

Jump to bottom

Fix#22 #24

Merged
merged 9 commits into from
Jun 21, 2021
Merged

Fix#22 #24

merged 9 commits into from
Jun 21, 2021

Conversation

chrstnbwnkl
Copy link
Contributor

  • Changed Router base class to be BaseClient base class
  • Added Client class derived from BaseClient that uses requests
  • Updated all routers to have clientargument with Clientas default value

Caveats:

  • args passed to router not available via router instance, only through router's client instance (e.g. router.client.proxies instead of router.proxies)

@nilsnolde
Copy link
Owner

Can you merge master into this branch? Only changed formatting stuff. Also, don't forget to re-install the pre-commit hook with pre-commit install.

@chrstnbwnkl
Merge branch 'master' into fix#22
7423198 
# Conflicts:
#	routingpy/client_default.py
#	routingpy/routers/google.py
#	routingpy/routers/graphhopper.py
#	routingpy/routers/heremaps.py
#	routingpy/routers/mapbox_valhalla.py
#	routingpy/routers/openrouteservice.py
#	routingpy/routers/osrm.py
#	routingpy/routers/valhalla.py
#	tests/test_base.py
@nilsnolde nilsnolde self-requested a review June 21, 2021 10:17
nilsnolde
nilsnolde requested changes Jun 21, 2021
View reviewed changes
Copy link
Owner

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

Thanks for the first shot! Left a few todos, call me if anything unclear.

routingpy/base.py Outdated Show resolved Hide resolved
routingpy/base.py Outdated Show resolved Hide resolved
routingpy/base.py Outdated Show resolved Hide resolved
routingpy/base.py Outdated Show resolved Hide resolved
routingpy/client_default.py Outdated Show resolved Hide resolved
routingpy/client_default.py Outdated Show resolved Hide resolved
routingpy/routers/google.py Outdated Show resolved Hide resolved
@nilsnolde
Copy link
Owner

smth is not quite right with the linting still.. tons of formatting changes in this PR. don't sweat it though, could also be a mismatch of CI and local stuff: your latest commit passed but it shouldn't have. we'll figure that out after we merged this PR, as long as CI is happy.

@chrstnbwnkl chrstnbwnkl marked this pull request as ready for review June 21, 2021 13:46
@nilsnolde nilsnolde self-requested a review June 21, 2021 13:52
nilsnolde
nilsnolde requested changes Jun 21, 2021
View reviewed changes
Copy link
Owner

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

Almost there;) there's quite a few unrelated changes which didn't make sense. best you go through the diff here on github and revert all those.

Other than that, three more things:

  • please go through the docstrings a final time and update them where you changed things (lots of request_kwargs still there)
  • let's remove the client_kwargs arg to _request() entirely. I explained why in the comment.
  • the tests are still failing, but all with the same error. See if you can get the reason, else we'll look into tmrw morning together.

routingpy/client_base.py Outdated Show resolved Hide resolved
routingpy/routers/google.py Outdated Show resolved Hide resolved
routingpy/routers/google.py Outdated Show resolved Hide resolved
routingpy/routers/graphhopper.py Outdated Show resolved Hide resolved
routingpy/routers/graphhopper.py Show resolved Hide resolved
routingpy/routers/heremaps.py Outdated Show resolved Hide resolved
routingpy/routers/mapbox_valhalla.py Outdated Show resolved Hide resolved
routingpy/routers/openrouteservice.py Outdated Show resolved Hide resolved
routingpy/routers/osrm.py Outdated Show resolved Hide resolved
routingpy/routers/valhalla.py Outdated Show resolved Hide resolved
@nilsnolde nilsnolde self-requested a review June 21, 2021 16:30
nilsnolde added 2 commits June 21, 2021 18:59
@nilsnolde
fix tests: apparently there was both a bug in the here tests and a bu…
335926d 
…g in responses (since it didn't pick up on our bug before the last PR's packages updates)
@nilsnolde
add requests URL to docstring
7c44586
nilsnolde
nilsnolde approved these changes Jun 21, 2021
View reviewed changes
Copy link
Owner

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

found the bug: wasn't your doing. apparently we had a small bug in the tests where it called the wrong URL. and then there was another bug in responses which caused our bug to not be detected. What's weird there: I updated the dependencies in the last PR (which must've caused that it failed now), but CI went through..

Anyways, I quickly updated that, for me this is good to go. Thanks @chrstnbwnkl

@nilsnolde nilsnolde merged commit e6add3d into master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilsnolde nilsnolde nilsnolde approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrstnbwnkl @nilsnolde