-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Check python pin
compatibility with Requires-Python
#4989
Conversation
ab6ab5b
to
f8cfdcf
Compare
I took a closer look and have some more substantive requested changes. Let me know if you have questions or if the scope is too big. |
5bf20d9
to
107735d
Compare
a377c35
to
6eaa07d
Compare
Ah terrifying you force pushed while I was editing locally :) |
I'll make sure this is merged today |
AHH! Sorry, was trying the pull in main and fix the merge conflicts. You can take over the branch :) |
No problem haha it's your pull request! I'll take care of the remaining conflicts though :) thanks |
Thanks for being patient with this one! I made some important fixes as well as some simplifications the code and tweaks to messages — we could probably make factor the code into something further easier to read but it seems decent for 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.
Awsome!
Thanks for the thorough review! Appreciate it |
Updated `.python-version` from `[email protected]` -> `cpython` | ||
|
||
----- stderr ----- | ||
warning: The requested Python version `cpython` resolves to `3.10.[X]` which is incompatible with the project `Requires-Python` requirement of `>=3.12`. |
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.
Am I misunderstood or cpython
must be resolved to 3.11? since context already contains it.
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.
ref #5205
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.
Yes that's problematic. cc @charliermarsh looks like this regressed.
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.
This actually isn't a regression or related to #5205, we're just taking the first Python on the path. The problem here is that we need to respect Requires-Python
on top of pinned version requests (😭).
Summary
Resolves #4969
Test Plan
cargo test
and manual tests.