-
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
DO NOT sort the returned matches by version #8319
DO NOT sort the returned matches by version #8319
Conversation
A higher version is not always preferred over the lower; the user may be explicitly preferring lower versions by specifying --prefer-binary or similar flags. PackageFinder already takes these into account for these and orders the matches. Don't break it.
if c.is_installed and not _eligible_for_upgrade(c.name): | ||
return (1, c.version) | ||
|
||
return (0, c.version) | ||
return 1 | ||
return 0 |
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.
nit:
return int(c.is_installed and not _eligible_for_upgrade(c.name))
puppy face
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 nit-picking, but this change LGTM.
This is likely also the fix to the currently xfail-ed |
I'm gonna go ahead and merge this without Paul's green tick since... I'm pretty certain that this change is good to go. (please holler if I'm wrong Paul!) |
Fine with me, I've been mostly offline all day (public holiday in the UK, plus attempting to take some time away from a PC screen). |
Bug found by the unit test introduced in #7996.
A higher version is not always preferred over the lower; the user may be explicitly preferring lower versions by specifying --prefer-binary or similar flags.
PackageFinder
already takes these into account and orders the matches. Don't break it.