-
Notifications
You must be signed in to change notification settings - Fork 913
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
Use current and requested Python versions in requires-python
incompatibility errors
#986
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
a0c1164
Use current Python version in `requires-python` incompatibility errors
zanieb eeadecd
Attach Python requirements to no solution errors to improve messaging
zanieb f071a03
Do a simpler comparison instead of extending `Version`
zanieb 35b2b88
Merge branch 'main' into zb/python
zanieb 066b800
Drop extra version display (was from debugging)
zanieb 4a5a1a5
Lint
zanieb 334dc1e
Fix typo
zanieb 83bda66
Use owned versions in `PythonRequirement`
zanieb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they not exactly the same when not provided? I think I would've expected
--python-version 3.10
to be treated as--python-version 3.10.0
. I'm just wondering if this will be correct if the user uses a different patch version in the resolution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a target Python version is provided e.g.
--python-version 3.11
If the target Python version is inferred from the current version
If a target Python version is provided with patch version e.g.
--python-version 3.11.3
If a target Python version is provided but not installed e.g.
--python-version 3.13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We compare the full target to the first two numbers of the installed so we should only have equality here when
--python-version
is not provided.Let me know if I can make the this clearer in my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are helpful...
(So first, I am assuming that
3.12
and3.12.0
are considered equal when you use==
withVersion
. I'm pretty sure this is true and per spec. In that case, we shouldn't need to do the.take(2)
thing.)I initially thought this one was a bug, because 3.11 should've resolved to 3.11.0. But then I realized we do a thing whereby we assume the highest known minor. That behavior kind of feels wrong in hindsight... I think it exists because
--python-version 3.7
would otherwise use3.7.0
, and a lot of things aren't compatible with the first few patch releases in3.7
, and that was a confusing user experience in my testing. But it might be wrong...So these should match based on
Version
equality, I think.These should be considered unequal based on
Version
equality.This seems right to me, they'd be considered unequal based on version equality. But it shouldn't have to do with whether the target is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which case specifically would be wrong if we used
==
instead oftake(2)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we see
3.12.1 != 3.12
though? Why is one of the versions not inclusive of the patch, when you don't provide a--python-version
? Isn't that wrong?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why it's implemented that way, but yes as noted above if the target version is inferred from the current Python version it will not include the patch version. I'm all for changing that behavior if it was not intentional, but it doesn't seem like we should do it here.
Here's another commit showing the installed version alongside the target one: 43910a8e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think the right order of operations is to fix the missing patch version (in a separate PR) and then rebase this on top of that. Otherwise, we're adding a workaround for a bug that we need to fix immediately. But feel free to merge, I see the issue and can fix now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tossed something up here 🤞 #1033
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing it!