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

version: fix local label comparison #379

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

abn
Copy link
Member

@abn abn commented May 26, 2022

Prior to this change, local label comparison was inconsistent with PEP 440. This change ensures that the local version label is checked for equivalence using a strict string equality comparison.

Resolves: python-poetry/poetry#4729

@abn abn requested a review from a team May 26, 2022 15:54
abn added 2 commits May 26, 2022 18:26
Prior to this change, local label comparison was inconsistent with
PEP 440. This change ensures that the local version label is checked
for equivalence using a strict string equality comparison.

Resolves: python-poetry/poetry#4729
@abn abn force-pushed the fix-local-version-comparisons branch from cfdccfc to c4bb9bd Compare May 26, 2022 16:26
@sonarcloud
Copy link

sonarcloud bot commented May 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

I don't think that's completely correct yet. PEP440 says:

"Comparison and ordering of local versions considers each segment of the local version (divided by a .) separately. If a segment consists entirely of ASCII digits then that section should be considered an integer for comparison purposes and if a segment contains any ASCII letters then that segment is compared lexicographically with case insensitivity. When comparing a numeric and lexicographic segment, the numeric section always compares as greater than the lexicographic segment. Additionally a local version with a great number of segments will always compare as greater than a local version with fewer segments, as long as the shorter local version’s segments match the beginning of the longer local version’s segments exactly."

It seems that comparing local version segments is already prepared here:

_local: tuple[tuple[int, int | str], ...]
if self.local is None:
# Versions without a local segment should sort before those with one.
_local = ((NegativeInfinity(), ""),)
else:
# Versions with a local segment need that segment parsed to implement
# the sorting rules in PEP440.
# - Alpha numeric segments sort before numeric segments
# - Alpha numeric segments sort lexicographically
# - Numeric segments sort numerically
# - Shorter versions sort before longer versions when the prefixes
# match exactly
assert isinstance(self.local, tuple)
_local = tuple(
# We typecast strings that are integers so that they can be compared
(int(i), "") if str(i).isnumeric() else (NegativeInfinity(), i)
for i in self.local
)

Comment on lines +131 to 132
assert v.allows(Version.parse("1.2.3-1+build.1"))
assert v.allows(Version.parse("1.2.3-1+build.1"))
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated line.

Maybe, it's worth adding assert not v.allows(Version.parse("1.2.3")) since that's new.

@@ -69,6 +69,11 @@ def is_simple(self) -> bool:

def allows(self, other: Version) -> bool:
if self._min is not None:
if self._min.is_local() and (
not other.is_local() or self._min.local != other.local
Copy link
Member

@radoering radoering May 29, 2022

Choose a reason for hiding this comment

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

That's not completely correct. It actually has to be something like self._min.local < other.local. (However, this falls too short.)

Copy link
Member Author

@abn abn May 29, 2022

Choose a reason for hiding this comment

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

IIRC, local labels are to be treated as exact strings, so they should not be ordered when comparing.

Copy link
Member Author

@abn abn May 29, 2022

Choose a reason for hiding this comment

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

image

🤣

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it might not be suitable for stuff like 1.10.0+cu113 and 1.10.0+cpu but that's what PEP440 says.

@@ -81,6 +86,10 @@ def allows(self, other: Version) -> bool:
if not _this.is_local() and _other.is_local():
# allow weak equality to allow `3.0.0+local.1` for `<=3.0.0`
_other = _other.without_local()
elif _this.is_local() and (
not _other.is_local() or _this.local != _other.local
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@dimbleby
Copy link
Contributor

dimbleby commented Jun 7, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version matching bug (or feature)?
3 participants