Skip to content

wip: finish PEP508/PEP440 impl for version matching #2856

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

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented May 3, 2025

Implement the full PEP440 and PEP508 version comparison and handle the
platform_version being special (i.e. it can be a version or a string).

This is still WIP, but feel free to grab it and work on it.

Work towards #2826

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

I factored a check_evaluation helper out.

Also added a test to just loop over (expr, want, env) cases. I figure we can copy/paste some real expressions. Also makes it easy to add some arbitrary expression to test.

@rickeylev
Copy link
Collaborator

I tried fixing a bug with prefix matching exposed by the extra tests. I'm not entirely sure my revision is entirely correct. For fun, I ran the question through Gemini, it took like minutes (:astonished:) of thinking, and ultimately came back with something resembling:

if left and right are prefix: ...
elif left is prefix: ...
elif right is prefix: ...
else: ...

Which satisfies my confirmation bias, but I didn't look closely or think it through, so yeah, grain of salt.

Also, for version equality, the local part should be ignored, so I commented out that part of _version_eq

https://packaging.python.org/en/latest/specifications/version-specifiers/#id5

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.

I couldn't find an "except where noted" later, so I'm pretty sure the '+blabla` is supposed to be ignored.

@aignas
Copy link
Collaborator Author

aignas commented May 4, 2025

Oh nice, thank you, I will check it my tomorrow. I knew that the prefix matching is not fully done yet, but ran out of time yesterday.

@aignas
Copy link
Collaborator Author

aignas commented May 5, 2025

I realized that the prefix matching could be a simple string prefix match so I went that way, the code became easier to reason about and I think it needs to be optimized a little, because I am normalizing the version twice, but I think this direction is promising. Ran out of time today, so I'll leave it for the next time.

@aignas
Copy link
Collaborator Author

aignas commented May 6, 2025

It seems that this is still not done. I am not sure I can fully grok the spec about what we should do with the version ordering.

The exclusive ordered comparison
https://peps.python.org/pep-0440/#exclusive-ordered-comparison

is talking about pre-release versions, but I am not sure if that applies to the dev and local versions. That said, that part of the pep is about version specifiers and not necessarily comparison of two versions.

Maybe the ordering is answered in
https://peps.python.org/pep-0440/#exclusive-ordered-comparison

@aignas
Copy link
Collaborator Author

aignas commented May 6, 2025

There is a remaining test case that we need to fix and it is about ensuring that "3.12.0" is equivalent to "3.12" in the expressions so that the equality operator works.

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.

2 participants