-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat: router for OpenTripPlanner #109
Conversation
khamaileon
commented
Jun 27, 2023
•
edited
Loading
edited
f24167b
to
83c3c3b
Compare
83c3c3b
to
a1a9859
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @khamaileon , very good effort! You're the first one actually contributing a whole provider 💯
This seems to target OTP v1.x right? I had little exposure to their API, but I do see an isochrone implementation and AFAIK that was dropped in v2.x (though I heard that it'll be re-implemented due to popular demand or maybe it was already?). In that case we should probably name this provider OpenTripPlannerV1
(or OpenTripPlannerV2
respectively, v1 is still in wide use)?
I don't have much to review on the API side, just a few conceptual questions and nits. Since we didn't merge the "live request" support yet, I'd trust you there and handle bugs as they appear.
Thanks you 🙂. This is only a first draft. I've already identified few improvements. It's v2. You're right, it's better to specify it. Isochrone is implemented. Sorry, I had force-pushed and maybe you've only seen the first draft. |
98e4206
to
fcb4b67
Compare
Sure, no worries, I thought it was done for you. I converted your PR into a draft PR so it's clearer. Just un-draft it once you're ready:) Thanks! |
Ah I thought I'd drafted it. Hence the misunderstanding thank you! Okay, it's ready now :) |
Sorry for the delay on this, I’ll have a look until the end of the week. |
No worries, ty @nilsnolde |
routingpy/routers/__init__.py
Outdated
"here": HereMaps, | ||
"heremaps": HereMaps, | ||
"openrouteservice": ORS, | ||
"opentripplanner_v2": OpenTripPlannerV2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could allow shortcutting to otp_v2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
Thanks for the review. I'll let you do the merge :) |
Interesting, Github changed smth, usually it'd require a re-approve. Maybe bcs only the commented file changed.. Anyways, thanks, this is great! |