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

Add tests, usability adjustments and test data #9

Closed whytro closed 1 year ago

whytro commented 1 year ago

Added the bulk of tests, with reference to the OSRM NodeJS tests. In addition, precompiled test data has been added for now, to run the tests off of. Some changes have also been made to improve usability (intuitiveness) of the code.

whytro commented 1 year ago

Some of the bits I'm working on, or need to work on (for now):

The issue I'm currently trying to figure out is that for ResultT bindings, the bindings to the variant uses a call to the copy constructor of FlatbufferBuilder, which is deleted. I think I may opt for just directly returning the result type rather than binding and having the ResultT object accessible in the end.

nilsnolde commented 1 year ago

Thanks for the update! Looks good so far.

I think you can mostly leave it where it is for now, it's already quite big. At least if the tests pass locally. Do they?

The overarching Python wrapper, for further usability improvements (ie. so that the user does not need to know the specific call for AnnotationsType)

Let's do that in a separate PR.

Cut down on code repetition within kwarg handling

Same here.

Bindings for ResultT (Mapbox Variant of a JSON object, string or Flatbuffer)

I think we said we'll go with only JSON for now no? Let's put Flatbuffer on the nice-to-have TODO list. Not sure what string output is, but doesn't sound very useful. Can you give a quick example?

After this is merged, we should focus first on Github Actions to make sure we don't introduce any regressions going forward. Now we're almost finished with the API, let's protect that state:)

whytro commented 1 year ago

Yes, the tests all pass locally, provided that osrm-datastore is run prior.

I've made a few more adjustments in the above commit - I think that some type bindings are redundant, and can better be represented as strings externally, while it converts those strings to the appropriate types internally.

As for repetitive kwargs, my current thought process is to return to a similar approach to the EngineConfig kwargs handling. I'm not sure if the addition of an additional Python wrapper will cut down on it, since the params still need to be passed in somehow.

As for ResultT, while we've said that we'll only go with JSON for now, Tile only returns a string output or a ResultT output. I was briefly working on that when the results still came with an "ok" attribute, so as to be able to attach the error/code. I re-shelved it again for now because of the approach to just throw an exception containing the code & message upon error rather than have the ok attribute + code/error as an object inside a ResultT.

Agreed on Github Actions being an important next step!

whytro commented 1 year ago

I had to overhaul to string implicit conversions a little - while I initially said that we could remove some of the "internal" types that likely would never see use, I realized how inflexible the previous solution was. For example, while it was able to accept a string instead of a specific enum as a constructor parameter, it would not work on variable assignment (ie. route_params.overview = "full"), but also not represent itself a string (ie. print(route_params.overview) would have printed out an object type rather than "full").

Tacking onto the previous solution would've resulted in some really ugly and annoying to maintain code (and I wasn't really happy with the previous solution), so I opted to go for a map across the board, with the __repr__ call returning the key of the value in the map - so any future additions or changes would only require one point of change.

The current test set still passes without issue, and the only thing I changed in this commit was to reflect that variable assignment can work implicitly with strings (such as route_params.overview = "full").

In the process, I've also centralized the handling of RouteParameters and BaseParameters variable assignments to make them easier to maintain and modify.

whytro commented 1 year ago

I've adjusted the str_to_enum exception message to mention valid value strings for usability.

The incomplete tiles return value is also fixed - I realized that nb::bytes converts a null-terminated C-style string into bytes, so adding the size value cleared up the issue (it was probably "terminating early" without).