Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Inform users when vcs's are not in the path. #4461

Merged
merged 9 commits into from May 3, 2017
Merged

Inform users when vcs's are not in the path. #4461

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9a37000
Inform windows users when vcs's are not in the path.
AraHaan May 2, 2017
f5ba12f
Create 4461.bugfix
AraHaan May 2, 2017
aafdb4c
Update __init__.py
AraHaan May 2, 2017
947c3c1
Update __init__.py
AraHaan May 2, 2017
87a19dd
Update 4461.bugfix
AraHaan May 2, 2017
cb1227e
Update __init__.py
AraHaan May 2, 2017
1a9eac3
Update 4461.bugfix
AraHaan May 2, 2017
1f2df74
Update 4461.bugfix
AraHaan May 2, 2017
737ac35
Fix formatting.
AraHaan May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/4461.bugfix
View file
Open in desktop
@@ -0,0 +1,2 @@
This fix clarifies the error given to suggest to the user
what might be wrong.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean literally just that text :-( It was just a suggestion on the way to express what had changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh

5 changes: 4 additions & 1 deletion pip/vcs/__init__.py
View file
Open in desktop
Expand Up @@ -327,7 +327,10 @@ def run_command(self, cmd, show_stdout=True, cwd=None,
# errno.ENOENT = no such file or directory
# In other words, the VCS executable isn't available
if e.errno == errno.ENOENT:
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 % self.name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...".

raise # re-raise exception if a different error occurred

Expand Down