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

Fallback to su if sudo doesn't exist #1006

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

moore3071
Copy link

su should exist even if sudo doesn't. As such, we can try to run a command with su if sudo doesn't exist.

@scorphus
Copy link
Collaborator

Thanks for contributing. Can you please add tests?

@moore3071
Copy link
Author

Will do. It might not be until next week though.

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.

Please regard my change requests.

@pytest.mark.parametrize('output', [
'command not found: sudo'])
def test_match(output):
assert match(Command('', output))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good having some practical test cases. command.script_parts is part of the match condition but not taken into consideration in tests.

assert not match(Command('', ''))
assert not match(Command('sudo ls', 'Permission denied'))
assert not match(Command('su -c ls', 'Permission denied'))
assert not match(Command('ls', 'command not found: ls'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ pytest.mark.parametrize is being used in test_match for one entry and not in test_not_match for four entries. Use it here as well.

@@ -0,0 +1,12 @@
def match(command):
Copy link
Collaborator

Choose a reason for hiding this comment

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

thefuck.utils.for_app is there for the help \o/

Suggested change
def match(command):
from thefuck.utils import for_app
@for_app('sudo')
def match(command):

@@ -0,0 +1,12 @@
def match(command):
if command.script_parts and '&&' not in command.script_parts and command.script_parts[0] == 'su':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if command.script_parts and '&&' not in command.script_parts and command.script_parts[0] == 'su':
if '&&' not in command.script_parts:

Also, Fish Shell must be considered. Support to && operator has been added to version 3. Earlier versions only support the and operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

sudo.py suffers with the same issue, and that's already released.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's true. I's been sitting there unnoticed. Thanks, @djh82. That must be fixed.

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.

None yet

3 participants