-
Notifications
You must be signed in to change notification settings - Fork 3.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
Resolve direct and pinned requirements first #9211
Conversation
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'm not 100% sure how _get_restrictive_rating
implements the order of preference described. That's mainly because I don't really understand what get_candidate_lookup
does, though, so it's not directly an issue with this PR.
We should maybe add a documentation comment for get_candidate_lookup
, and explain a bit better how it links to the preference calculation here, but that can easily be a followup (it's definitely not the worst documented bit of code in pip!)
289488b
to
2e13758
Compare
Good idea, added! |
Awesome :-) Switching to an explicit check rather than using The subtle difference between |
This in turn prompted me to search the code base for pip/src/pip/_internal/operations/prepare.py Lines 412 to 418 in e6da461
And I… have doubts if the implementation is correct. |
2e13758
to
4ad924a
Compare
Yeah, that looks weird. This is why I tend to leave the hash-checking code to people who use/need the feature (and therefore presumably understand the requirements 😉) |
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.
One "I'd really like to wrap this up in one go" suggestion: #9211 (comment)
Other than that, this looks great!
Linters aren't happy, but happy to merge once that's fixed. |
1f2c8cd
to
82fe333
Compare
Close #9185.