Closed tmadlener closed 1 month ago
Unless, there are any major concerns here, I would like to merge this later today and make a v01-01 tag tomorrow, once the nightlies have passed.
Reading between the lines the bindings for Version
from cppyy had broken comparisons magic methods, right?
Is it because cppyy doesn't handle <=>
?
Wouldn't it be enough in python to just give a version as a string? And in the client code just use packaging.version.parse
which is pretty standard?
Reading between the lines the bindings for
Version
from cppyy had broken comparisons magic methods, right? Is it because cppyy doesn't handle<=>
?
You are correct. I didn't check whether it's actually the <=>
that isn't detected or whether it's the fact that we default it.
Wouldn't it be enough in python to just give a version as a string? And in the client code just use packaging.version.parse which is pretty standard?
We could also do that, but I am not sure how many physics users are aware of that ;) (I wasn't until an hour ago)
You are correct. I didn't check whether it's actually the
<=>
that isn't detected or whether it's the fact that we default it.
I think it's just any <=>
since cppyy is C++17 :cry:
Using manually defined comparison operators works fine.
Maybe we could just remove the C++20 version from#if
:sob:
We could also do that, but I am not sure how many physics users are aware of that ;) (I wasn't until an hour ago)
Fair point. It just feels like a lot of code just to wrap the packaging.version
in the end
One other issue is that cppyy can apparently not deal with aggregate initialization, because simply doing
if ROOT.gInterpreter.LoadFile("podio/podioVersion.h") == 0: # noqa: E402
from ROOT import podio # noqa: E402 # pylint: disable=wrong-import-position
Version = podio.version.Version
v = Version(1, 2, 3)
leads to:
TypeError: none of the 3 overloaded methods succeeded. Full details:
Version::Version(podio::version::Version&&) =>
TypeError: takes at most 1 arguments (3 given)
Version::Version() =>
TypeError: takes at most 0 arguments (3 given)
Version::Version(const podio::version::Version&) =>
TypeError: takes at most 1 arguments (3 given)
In that case, I would rather keep the c++ version unchanged and do a bit more legwork on the python side, even if we effectively just re-wrap packaging.version.Version
again.
Ouch, okay then :+1: I'm not sure if this isn't an error in ROOT cppyy, I'll ask later
I added an alternative implementation where we effectively leverage the c++ class and only add a new parse
method as well as a shim for __str__
to get nicer print outputs. In the end the amount of code is pretty similar, considering that now a bunch more tests have to be added for version.parse
. Any strong preferences for any of the two implementations?
No strong preferences. This version doesn't need the packaging
in requirements and readme anymore :wink:
Dropped the commits with the changes to requirements.txt
. I am going with this one. If cppyy starts supporting operator<=>
and aggregate initialization, we should be able to pick that up transparently.
Aggregation init issue root-project/root#16469
BEGINRELEASENOTES
podio.version
module exposingpodio::version::Version
also in python.version_as_str
method again, since that is handled byVersion
now.ENDRELEASENOTES