fix(pep440): do not ignore post for inclusive ordering

[abn]

Summary by Sourcery

Bug Fixes:

• Fixed an issue where inclusive ordering with post releases were inconsistent with PEP 440.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[radoering]

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:

poetry-core/src/poetry/core/version/pep440/version.py

Lines 108 to 125 in dfd1b39

	_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
	)

[dimbleby]

python-poetry/poetry#5791 (comment)

[abn]

@sourcery-ai review

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request fixes an issue where local label comparison was inconsistent with PEP 440. The fix ensures that post releases are not ignored when performing inclusive ordering comparisons.

Class diagram showing changes to VersionRange class

classDiagram
    class VersionRange {
        -_min: Version
        -_max: Version
        -_include_min: bool
        -_include_max: bool
        +is_any(): bool
        +is_simple(): bool
        +is_inclusive(): bool
        +is_exclusive(): bool
        +allows(other: Version): bool
    }
    note for VersionRange "Added new properties is_inclusive and is_exclusive
Modified allows() method to handle post-releases correctly"

Loading

Flow diagram for version comparison logic

flowchart TD
    A[Start Version Comparison] --> B{Is minimum version set?}
    B -->|No| E[Allow version]
    B -->|Yes| C{Is exclusive comparison?}
    C -->|No| E
    C -->|Yes| D{Is other version a post-release<br>and min version not?}
    D -->|Yes| F[Reject version]
    D -->|No| E

Loading

File-Level Changes

Change	Details	Files
Fixed an issue where local label comparison was inconsistent with PEP 440.	Added parameterized tests to verify inclusive ordering with post and local releases. Removed old tests that were not parameterized.	tests/constraints/version/test_version_range.py
Ensure that post releases are not ignored when performing inclusive ordering comparisons.	Added is_inclusive and is_exclusive properties to the VersionRange class. Modified the allows method to correctly handle post releases when performing exclusive ordered comparisons.	src/poetry/core/constraints/version/version_range.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time. You can also use
  this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @abn - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[radoering]

Actually, I am not sure if this fixes anything regarding compliance with PEP 440. I even think this will be less compliant with PEP 440 than before.

It feels like switching from one use case to another. According to PEP 440, local version labels are not allowed in constraints with >, <, >=, <=. However, since we are building version ranges, we have to deal with it anyway. Although they are not allowed in constraints, it is defined how they are ordered and compared, see #379 (review)

If I understand correctly, this PR tries to make something like this work:
>= 1.0+cpu does not allow 1.0+cuda or 1.1+cuda (even though they are both greater!) but 1.1+cpu (which is also greater)

However, to make this work, it breaks the use case where >=1.0+internal.1 allows 1.0+internal.2.

Honestly, the second use case feels more correct (i.e. more compliant) than the first one. On the other side, I understand that the first one might be the more common one (at least when neglecting private packages inside of companies).

I think this change will be breaking for some users and make us less compliant with PEP 440. I feel like we should carefully weigh if it is worth it. Is there an open issue about this? The issue that is linked has already been resolved.

[abn]

When through the spec again, been a while. And I agree that the original changes I had is swapping one "interpretation" for another. I also agree that >=1.0+internal.1 allows 1.0+internal.2 is better than >= 1.0+cpu does not allow 1.0+cuda as the latter does not comply with if a segment contains any ASCII letters then that segment is compared lexicographically with case insensitivity.

Taking all that into considering, I have gone ahead and repurposed this PR to only include the following:

1. Test cleanups and new test cases.
2. Fixing inclusive ordering.

As for what to do with cpu vs cuda, I am more convinced that solving this at the version range level is not the appropriate fix if we want to ensure that VersionRange.allows adheres to PEP440 inclusive and exclusive ordering. We do bend the rules (see quote below) a bit already because we have to consider local versions.

Except where specifically noted below, local version identifiers MUST NOT be permitted in version specifiers, and local version labels MUST be ignored entirely when checking if candidate versions match a given version specifier.

[abn]

@sourcery-ai review

Out of date.

[sourcery-ai]

Hey @abn - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟢 Complexity: all looks good
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[radoering]

Looks like a valid fix now. 😄 Just some nitpicks.