Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make LooseVersion('1.0') == LooseVersion('1') #4691

Merged
merged 7 commits into from
Nov 22, 2024

Conversation

Flamefire
Copy link
Contributor

Remove the pitfall where missing components were filled with empty strings that are less than any other value.

The common expectation is that trailing zeroes are equal to zeroes.
I don't think there is any case where 1.0 > 1 is intended

If the trailing part is a string the current behavior is kept.

See the changed test case where "# Careful here: 1.0 > 1 !!!" can be removed now.

Remove the pitfall where missing components were filled with
empty strings that are less than any other value.
The common expectation is that trailing zeroes are equal to zeroes.
If the trailing part is a string the current behavior is kept.
@Flamefire Flamefire force-pushed the looseversion branch 3 times, most recently from 865094f to 60cc742 Compare October 24, 2024 14:00
Currently there is no way to detect whether a version was defaulted.
The current default value ('0.0.0') is a valid value for the versions
list and the appropriate default comparator for that should be `==`.
However such a version is (mis-)detected as being the default and the
resulting default comparator is `>=` which is less restrictive than intended.

Fix by introducing a property set only for the default version.
The default_version `1.0` no longer matches `> 1` so raise to 1.1
@akesandgren
Copy link
Contributor

I bet that some stupid developer will make version 1 and then produce version 1.0 as being newer then 1...
And then @boegel will have more material for his talk...

@Flamefire
Copy link
Contributor Author

I bet that some stupid developer will make version 1 and then produce version 1.0 as being newer then 1... And then @boegel will have more material for his talk...

Sure. But technically 1.0-alpha < 1.0 but that isn't the case for LooseVersion either (it is the reverse), and on purpose hence the "Loose".
I'd argue that us expecting version > LooseVersion("1") holds for version=1.0 and 1.0.0 is more important to meet

The documentation:

When comparing version numbers, the numeric components will be compared numerically, [...]
the rules for comparison are simple and predictable, but may not always give the results you want (for some definition of "want").

to me implies 1.0 != 1 is not as simple and predictable. But I guess that's opinionated too.
If that is disagreed on I'll drop that again, but I'd keep the other fix to the EasyConfigObj in a separate PR

@boegel boegel added bug fix EasyBuild-5.0 EasyBuild 5.0 labels Nov 6, 2024
@boegel boegel added this to the 5.0 milestone Nov 6, 2024
Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of any situations where this actually has mattered, but i'm fine with the logic change.

@lexming
Copy link
Contributor

lexming commented Nov 22, 2024

I agree with the premise that 1 should always be considered at the same level as 1.0. But technically, according to PEP440, version 1 is indeed lower than 1.0. See https://peps.python.org/pep-0440/#summary-of-permitted-suffixes-and-relative-ordering

You can have development and alpha versions like 1.dev0 in between 1 and 1.0 for instance.

@Micket
Copy link
Contributor

Micket commented Nov 22, 2024

I'd just add the note that we use this not really specific for python packages but for all kinds of software packages which probably don't feel a need to follow PEP440 (but they also probably don't feel a need to follow semver or any other guides and just do whatever the heck they want)

If a develop actually make a 1 release, then follow that up with a 1.dev0 and a final 1.0 release they are just evil.

I thinking it's mostly going to be us that might use the single digit, for scenarios like

if self.version >= LooseVersion('7.0'):  # what if version is just 7?
     self.new_installation_step()
else:
     self.legacy_installation_step()

edit: uhm, i didn't think that example through, i hoped i fixed it to a maybe-realistic scenario.

@lexming
Copy link
Contributor

lexming commented Nov 22, 2024

As I said, I agree with this change. It is easier to dance around rare exotic versions than having to constantly handle 1 being different than 1.0. Just wanted to make sure that we are aware this breaks a PEP and we do this consciously.

@Flamefire
Copy link
Contributor Author

But technically, according to PEP440, version 1 is indeed lower than 1.0. See https://peps.python.org/pep-0440/#summary-of-permitted-suffixes-and-relative-ordering

I don't read it as such. The opposite: We fix this to match the described PEP:

All release segments involved in the comparison MUST be converted to a consistent length by padding shorter segments with zeros as needed.

You can have development and alpha versions like 1.dev0 in between 1 and 1.0 for instance.

My understanding of

Within a numeric release (1.0, 2.7.3), the following suffixes are permitted and MUST be ordered as shown:

.devN, aN, bN, rcN, , .postN

is that 1.dev0 < 1. and with the above 1. == 1.0. So I'd expect 1.dev0 < 1.0 which is in the table.

Our class doesn't follow that though: Because "dev0" > "0" we have 1.dev0 > 1 and equivalently1.dev0 > 1.0. But ours is a LooseVersion, not a SemVersion anyway.

@Micket How about:
if self.version >= LooseVersion('7.0'): # what if self.version is either 7 or 7.0.0

With this change they'd all be equal, i.e. True, before the former was false, the latter true if I'm not mistaken.

@Micket
Copy link
Contributor

Micket commented Nov 22, 2024

yes I'm in favour of this change, I tried to write an example where the old behavior was undesirable. I asked the other maintainers for opinions, and it doesn't seem like people have strong opinions or vaguely in favor, so in this goes.

@Micket Micket merged commit cad3801 into easybuilders:5.0.x Nov 22, 2024
39 checks passed
@Flamefire Flamefire deleted the looseversion branch November 22, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants