search

nilsnolde / py-osrm

Python bindings to the OSRM routing framework
https://gis-ops.github.io/py-osrm/
BSD 2-Clause "Simplified" License
8 stars 8 forks source link

Adding further engine bindings #6

Closed whytro closed 1 year ago

whytro commented 1 year ago

Added more bindings for the other engine calls. However, I did run into an issue with the MatchParameters bindings, which I suspect has something to do with the way the argument forwarding is set up.

Also added __repr__ outputs for the custom JSON types, so they print out as expected.

whytro commented 1 year ago

I've attached the MatchParameters file as reference here, but the gist of it is that I can't find the proper parameter combination to have it link. It seems to not like that waypoints is explicitly specified in MatchParameters, while it's also handled in RouteParameters, so it expects the waypoints parameters again, which also throws off the va starting point for BaseParameters.

nilsnolde commented 1 year ago

Thanks for the further work! It's quite a lot to review though;) It's ok for now, and let's merge once reviewed and got your small problem out of the way. After that, we should concentrate on testing IMO, which will also allow us to see how the API currently works for a user (it's hard to understand just looking at the code).

whytro commented 1 year ago

Thanks for the updates. Just a question for the JSON return type commit, then it LGTM. Sorry, if we were confusing on that, I'm just not sure your previous solution was actually working better for recursive types such as array/object.

Yes, it works both ways, and basic benchmarking seemed to show negligible difference between the two methods, so I went with the return visitor for code clarity.

On the topic of mapbox recursive types, it seems it works without an explicit .get() call, but I added it in anyways for clarity.

nilsnolde commented 1 year ago

LGTM, thanks!