effigies / looseversion

A backwards/forwards-compatible fork of distutils.version.LooseVersion
Other
13 stars 1 forks source link

RF: Split Python2 semantics into separate class #22

Closed effigies closed 12 months ago

effigies commented 12 months ago

17 was a bit of a mistake, as downstream tools (nipype is where I noticed it) relied on the TypeError to detect bad version strings. I have now created a LooseVersion2 class that gets Python 2 semantics back, but LooseVersion will revert to different semantics between Python 2 and 3.

effigies commented 12 months ago

cc @intgr

intgr commented 12 months ago

relied on the TypeError to detect bad version strings

Really though?

TypeError would not be thrown when parsing, but only when comparing a version to another version. And then it also depends on the other version being compared, they have to be similar enough to trigger the exception.

So seems to me like this "detection" would only work sometimes and in a surprising way. It's a mystery to me how anyone would find that behavior useful.

intgr commented 12 months ago

I mean fair enough, if you want to add a way for users to restore the buggy behavior.

But making this the "default" while at the same time keeping the now contradictory docstring, I think was a mistake.

If "bad versions" is indeed a useful concept, then definitely the docstring should be updated. The current docstring in looseversion 1.3.0 still lists a bunch of "valid version numbers" and explicitly says there is no such thing as an invalid version number.

https://github.com/effigies/looseversion/blob/e1a5a176a92dc6825deda4205c1be6d05e9ed352/src/looseversion/__init__.py#L106-L136

However, some pairs of comparisons in this list work, others throw TypeError. Which one of these is a "bad version"? What does "bad version" mean?

>>> LooseVersion('2.0b1pl0') < LooseVersion('11g')
True
>>> LooseVersion('2.2beta29') < LooseVersion('11g')
True
>>> LooseVersion('2g6') < LooseVersion('11g')
True
>>> LooseVersion('2g6') < LooseVersion('2.2beta29')
TypeError: '<' not supported between instances of 'str' and 'int'
effigies commented 12 months ago

Yes, we should probably update the docstring.