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.

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

Add rule for pip_install permission fix #895

Merged
merged 5 commits into from
Apr 24, 2019
Merged

Conversation

IngaFeick
Copy link
Contributor

This PR reacts to pip install permission issues by either adding --user to the command, or, if --user is already in the command, removing the --user flag and running the command with sudo.

@nvbn
Copy link
Owner

nvbn commented Apr 3, 2019

The rule looks good, thanks!

Can you please fix linter warnings?

./thefuck/rules/pip_install.py:14:10: E261 at least two spaces before inline comment
./thefuck/rules/pip_install.py:15:5: W191 indentation contains tabs
./thefuck/rules/pip_install.py:15:5: E101 indentation contains mixed spaces and tabs
./thefuck/rules/pip_install.py:15:68: W291 trailing whitespace

@scorphus
Copy link
Collaborator

scorphus commented Apr 4, 2019

Thanks for the contribution, @IngaFeick! I'd recommend spending some time configuring your editor to do things like strip trailing whitespaces, standardise indentation, lint the source code, etc.

@IngaFeick
Copy link
Contributor Author

IngaFeick commented Apr 8, 2019

@nvbn @scorphus sorry mates, I forgot to run flake before submitting 🤦🏻‍♂️
Should be fixed now

Copy link
Collaborator

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

Can you please change all occurrences of " to '? For sake of consistency :-)

def get_new_command(command):
if "--user" not in command.script: # add --user (attempt 1)
return command.script.replace(" install ", " install --user ")
else: # since --user didn't fix things, let's try sudo (attempt 2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This else here is unnecessary and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quotation has been changed as requested.
Could you kindly explain why the else is unnecessary? The script is supposed to react differently, depending on whether or not the command already included the --user flag, in which case sudo might fix the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

An else after a return is unnecessary and only increases indentation, decreasing readability.

This:

if condition:
    return foo
else:
    return bar

Is equivalent to this:

if condition:
    return foo
return bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦🏻‍♀️ you're absolutely right of course. I thought you asked me to delete the entire else block including the 2nd return statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as requested

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, pardon me for not being clear about that ☺️ Glad you changed it! Thanks for your contribution.

@nvbn
Copy link
Owner

nvbn commented Apr 24, 2019

Looks good, thanks!

@nvbn nvbn merged commit 82902fb into nvbn:master Apr 24, 2019
@IngaFeick
Copy link
Contributor Author

Great, thanks!

@hongshaoyang
Copy link

Hate to be a wet blanket, but I don't think running sudo with pip install is a good idea. Essentially, you're allowing arbitrary Python code from PyPI to be executed with system-wide admin privileges.

See this and this.

riley-martine pushed a commit to riley-martine/thefuck that referenced this pull request Dec 7, 2023
* Add rule for pip_install permission fix

* Fix whitespace

* Switch quotation to single

* remove 2nd else

* E261 indent comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants