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

Issue with optional types #28

Open whytro opened 1 year ago

whytro commented 1 year ago

This is a leftover issue from GSoC that I've been thinking about, and one that I think that should be fixed. In its current state, some bindings for optional types (ie. optional approaches) do not work properly - some cannot be used, or do not return a "tangible" value.

pybind11 offered an optional_caster so you could bind other optional types like boost::optional, but nanobind does not, and only has default bindings for the STL optionals.

After experimenting with overloading the optional bindings, I've started to think that it might be cleaner to knock out two issues at once by looking at converting boost::optional references in osrm-backend to their std::optional counterpart, though an initial test reveals that it might not be so simple due to the referred boost::spirit reliance on boost::optional, and while x3 might support std::optional, going from qi to x3 seems to require some rewriting of the parsing rules/grammar/etc.

nilsnolde commented 1 year ago

Hi again:) Great to have you still thinking about this. I didn't find the time yet to test stuff on all platforms, but that's still on my to do, then we also still need to pre-release and see that that works.

Regarding converting osrm-backend to std::optional, see https://github.com/Project-OSRM/osrm-backend/pull/6611. You came to a similar conclusion. If you're set to make this work, it seems to me your first approach might be faster (and less frustrating, albeit less ideal). @SiarheiFedartsou any opinion?

whytro commented 1 year ago

Hello! This issue was bugging me a little, so I've decided to try and fix it, for usability.

I've taken a look at the linked issue, but could not find any file changes related to upgrading to x3. While moving from boost::optional to std::optional is essentially a "slot-in" replacement, qi to x3 requires a bit more of a substantial rewrite because they've made some fairly major changes between the versions (ie. grammars have been replaced by groups of rules, some previously provided parsers have been removed in favor of user defined lambdas).

But, the first approach is something that I'm keeping in mind, since if I'm unable to move osrm-backend to std::optional in time, I figure that it's at least a good band-aid fix.