Inform users when vcs's are not in the path. #4461
Conversation
This fixes an issue where when someone who tries to use git with pop but pip can't because git is not in the path environment variable. This explicitly tells them how they can fix the issue.
pip/vcs/__init__.py
Outdated
| "rBot-devs/ffmpus/blob/master/pthmgr.e" | ||
| "xe and the source code can be found h" | ||
| "ere as well https://github.com/Decora" | ||
| "terBot-devs/pthmgr.".format(self.name)) |
There was a problem hiding this comment.
Strong -1 on the advert for pthmgr. Users can manage PATH in whatever way they like - it's not pip's job to advise on this.
You're also making the assumption that the user has the VCS installed but not in PATH - surely it's equally possible that they didn't install it? A much simpler change would be to just improve the message a little (on all platforms): "Cannot find command %r - do you have it installed and on PATH?"
pip/vcs/__init__.py
Outdated
| raise BadCommand('Cannot find command %r' % self.name) | ||
| raise BadCommand( | ||
| 'Cannot find command %r - do you have ' | ||
| '%r installed and in your PATH?' % self.name) | ||
| else: |
There was a problem hiding this comment.
You have 2 instances of %r but only one argument. Either add a second copy of self.name to the RHS or just go with "... do you have it installed ...".
news/4461.bugfix
Outdated
| This fixes an issue where when someone who tries to use git | ||
| with pip but pip can't because git is not in the path | ||
| environment variable. This explicitly tells them how they | ||
| can fix the issue. |
There was a problem hiding this comment.
Given the changed wording of the error, I'd reword this as well - something like "clarifies the error given to suggest to the user what might be wrong".
AraHaan
changed the title
news/4461.bugfix
Outdated
| @@ -0,0 +1,2 @@ | |||
| clarifies the error given to suggest to the user | |||
| what might be wrong. | |||
There was a problem hiding this comment.
I didn't mean literally just that text :-( It was just a suggestion on the way to express what had changed.
There was a problem hiding this comment.
Sorry for the repeated changes needed here to what's really a fairly simple changes. As the history has got quite messy, would you also be OK with squashing the PR before it's committed? Alternatively, with your permission, I could simply make a the error message change (crediting you in the NEWS entry, of course) on your behalf.
Regardless of your preference, thanks for taking the time to help with improving pip - and sorry for the nitpicking!
pip/vcs/__init__.py
Outdated
| raise BadCommand( | ||
| 'Cannot find command %r - do you have ' | ||
| '%r installed and in your ' | ||
| 'PATH?' % self.name % self.name) |
There was a problem hiding this comment.
You should not chain % operators like this. To supply multiple arguments to the % operator you need to supply a tuple on the RHS. But in this case I think that repeating the VCS command name is unnecessary - I'd just use "it" in place of the second %r. If you prefer the repeat, though, that's fine (just implement it correctly, please).
One question - did you test this? As it stands the code will fail. I was considering asking you to write a test for this, but it seemed like a lot of additional work for a simple change. But I'm now wondering.
news/4461.bugfix
Outdated
| @@ -0,0 +1,2 @@ | |||
| This fix clarifies the error given to suggest to the user | |||
| what might be wrong. | |||
There was a problem hiding this comment.
This still isn't wright, sorry - you need to think about how it would look in the larger NEWS file. There's no context here explaining which error message has been changed, or
What I was trying to suggest was that your original text:
This fixes an issue where when someone who tries to use git with pip but pip can't because git is not in the path
environment variable. This explicitly tells them how they can fix the issue.
needed to be changed because the revised error no longer explicitly told the user how to fix the issue, but rather suggested to the user how to check what the problem might be. So the text I suggested was intended as a possible replacement for the last sentence only.
|
I am alright with the change on my behalf and crediting me on it because I can't squash this PR on my iPhone and my laptop that I can do it on is not running and does not have internet unlike my iPhone. And so that is another reason why I didn't test my changes earlier to not know it would not work. Not only that but I am so used to python 3's string tuple formatting. I never really did the formatting like above before to really know. I just pushed an commit to attempt the fix to the formatting I did without using python 3's string formatting. Edit: right after sending this I now have a way to get my laptop online to squash this. It might take a few hours or Soto do it as I also plan to rename and edit the commit title and message. |
|
I just found out that Github has a "squash and merge" option. So you don't need to do the squash - I can do it for you. Sorry for the confusion, I didn't know that feature existed. Unless you have any further changes you want to make, I can merge this - it looks good to me now. Just let me know you're OK for it to go. |
|
Squash Merges are great, I mostly always use them now :) |
|
@dstufft Yeah, TIL :-) |
|
Alright I think it is good to go. I was thinking to make the commit match the PR's name and the message to match the contents in the 4461.bugfix. |
|
Thanks very much for this, and for your patience with my requests for edits! I hope I didn't make your first experience of contributing too painful :-) |
|
You did not make it bad at all. You did help with the suggestions. |
lock
bot
added
the
auto-locked
This fix clarifies the error given to suggest to the user what might be wrong.