Improve the presentation style of the pip upgrade prompt #10959
Conversation
pradyunsg
added
type: enhancement
pradyunsg
force-pushed
the
better-upgrade-warning
branch
from
32dcbd7 to
8d437b6
Compare
github-actions
bot
added
the
needs rebase or merge
pradyunsg
force-pushed
the
better-upgrade-warning
branch
from
03f1c63 to
138a5d8
Compare
pradyunsg
force-pushed
the
better-upgrade-warning
branch
from
78c04f7 to
a067d72
Compare
This makes it more reasonable to use a rich renderable for regular logging messages, instead of only using it for the diagnostic error messages.
This makes it possible to provide more terse suggestions that are still correct, depending on the user's environment.
- Move the lookup within `selfcheck/*.json` files into a method on `SelfCheckState`. - Factor out `PackageFinder` interaction into a separate function. - Rename variables to more clearly reflect what they're for. Co-Authored-By: Pradyun Gedam <[email protected]>
This style makes the upgrade prompt less of blurb of yellow text, making it easier to read while clarifying the presentation style to make it easier to figure out what the user is supposed to run. Co-Authored-By: Pradyun Gedam <[email protected]>
This would make it easier to diagnose what an issue might be, related to this logic, since the things that affect whether the message is printed, are now available in the debugging logs.
This makes it easier to test this code.
pradyunsg
force-pushed
the
better-upgrade-warning
branch
from
a067d72 to
4ec49ab
Compare
There was a problem hiding this comment.
A bunch of complaints here about edge cases where the "find a good invocation" logic could fail. However, to be honest, I'm completely fed up with the endless debates about "pip told me to run X and it didn't work", so if people are OK with this logic, and any flaws it might have, I'm not going to object.
The one thing I would like, though, is for us to explicitly state that whatever behaviour we choose is final, and has been revisited enough times, and we're no longer interested in PRs or issues that try to "fix" it.
Apart from the "simplify the suggestion" logic, I haven't looked too closely at the rest of the code, but the output looks great, so +1 from me on that aspect of the change.
|
|
||
| def get_best_invocation_for_this_pip() -> str: | ||
| """Try to figure out the best way to invoke pip in the current environment.""" | ||
| binary_directory = "Scripts" if WINDOWS else "bin" |
There was a problem hiding this comment.
I can't remember for certain - do we not support environments on Windows that use "bin" rather than "Scripts" (for example, MSYS2)? I note that pip._internal.locations.get_bin_prefix() allows both on Windows...
There was a problem hiding this comment.
I don't think we explicitly support them. None the less, if this fails, the thing that'd happen is a less-perfect prompt will be presented. :)
I've copied this over inspired by what venv does on 3.7 -- https://github.com/python/cpython/blob/3.7/Lib/venv/__init__.py#L114
| if exe_are_in_PATH: | ||
| for exe_name in _EXECUTABLE_NAMES: | ||
| if shutil.which(exe_name) == os.path.join(binary_prefix, exe_name): | ||
| return exe_name |
There was a problem hiding this comment.
I assume here that we don't care about cases where the user might, for example, have a shell alias of pip for some particular copy of pip - which would completely wreck all of the above logic.
There was a problem hiding this comment.
Pretty much. There's no way to detect or account for these things.
| # Try to use the basename, if it's the first executable. | ||
| found_executable = shutil.which(exe_name) | ||
| if found_executable and os.path.samefile(found_executable, exe): | ||
| return exe_name |
There was a problem hiding this comment.
This approach seems much more robust than the pip one, but still could be messed up by the shell alias problem.
There was a problem hiding this comment.
Yea, there's nothing we can do for shell aliases anyway. If things break, I think it's fair to say "you have an alias" as a cause for the issue.
This makes the tests more reliable and easier to reason about. This also adds a few more cases under the test conditions.
This addresses a review comment around the code style, and makes it possible to provide additonal context in the comment.
pradyunsg
force-pushed
the
better-upgrade-warning
branch
from
4ec49ab to
85ccf51
Compare
I'm happy to add a comment about this in this logic, but that would be a follow up PR though. I want to use this logic in the Windows protection logic (as noted in the first post), but am wary of how much scope-creep this PR has gotten already -- so there's at least one more change. These constant debates/discussions about the commands we print was basically what made me look at this code, and go "oh, we're doing the same thing in two places". I'd be a little sad if we don't consolidate them. :) |
+1 on this. |
|
I've added this into the 22.1 release milestone. If there's no blocking concerns, I'd like to get that into the release. I've checked that the release manager doesn't mind, although he is getting uncomfortable with the increasing frequency of me trying to get things in at the last minute. 😛 |
|
I'm going to go ahead and merge this -- if this breaks something in ways that are non-trivial to fix, I'll revert it. |
|
For posterity, adding pictures of before/after here: 
|
This has a bunch of nice things:
Manually tested with a bit of a hacky approach (modify the
__version__, do an install and test that install).An obvious follow up is to use
get_best_invocation_for_this_pythonon Windows, for thepip.exeprotection logic.