Add version module that allows to compare versions in python

[tmadlener]

BEGINRELEASENOTES

• Add a podio.version module exposing podio::version::Version also in python.
• Remove the version_as_str method again, since that is handled by Version now.

ENDRELEASENOTES

[tmadlener]

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.

[andresailer]

Looks good to me.

[m-fila]

Reading between the lines the bindings for Version from cppyy had broken comparisons magic methods, right?
Is it because cppyy doesn't handle <=>?

[m-fila]

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?

[tmadlener]

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)

[m-fila]

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 😢
Using manually defined comparison operators works fine.
Maybe we could just remove the C++20 version from#if 😭

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

[m-fila]

Should packaging also appear in the requirements.txt (and readme?).

[tmadlener]

Indeed it should. I though it was part of the standard library.

[tmadlener]

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.

[m-fila]

Ouch, okay then 👍
I'm not sure if this isn't an error in ROOT cppyy, I'll ask later

[tmadlener]

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?

[m-fila]

No strong preferences. This version doesn't need the packaging in requirements and readme anymore 😉

[tmadlener]

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.

[m-fila]

Aggregation init issue root-project/root#16469