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

Completions: add support for doas as for sudo #10256

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Completions: add support for doas as for sudo #10256

merged 2 commits into from
Sep 28, 2023

Conversation

ZerdoX-x
Copy link
Contributor

@ZerdoX-x ZerdoX-x commented Sep 6, 2023

Description

Fixes #2047 but for the doas command the same way as in #8094

Screenshots Fixes this difference:

image
image

User-Facing Changes

No breaking changes. If people not using doas, no difference at all.

Tests

I have not added any tests since its using same logic as for "sudo". I guess if something would go wrong in this part, sudo tests will cover it?

Additional context

As a nushell user I could not find a way to implement custom completion for a "sudo like command". Since I can see sudo being hardcoded in sources, this is what I propose.

Also I have almost zero knowledge of rust and this is definitely not the clean way yet

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Thanks for giving this a shot!
No fault for not being familiar with Rust yet. In this case your changes not yet add support for command completion after sudo/doas. In the screenshot you can see that it only suggests files (which I think it will fallback to quite liberally)

crates/nu-cli/src/completions/command_completions.rs Outdated Show resolved Hide resolved
crates/nu-cli/src/completions/completer.rs Outdated Show resolved Hide resolved
@ZerdoX-x
Copy link
Contributor Author

ZerdoX-x commented Sep 7, 2023

@sholderbach thank you so much for review <3

Screenshots are just illustration of the problem, not it's solution :D

I've made changes according to your code advice.

Treat doas as sudo while command completing.
Also adds "doas" to typos false positives.
@ZerdoX-x
Copy link
Contributor Author

@sholderbach Hi :)
I've resolved merge conflict and added doas to typos false positives so CI check would not complain.
Could you please take a look?

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this!

I think we should in the future have a way to simply provide a list of commands or a signature that will take external commands and arguments in the trailing position.

But to get this resolved I am fine with landing this right now so we can unblock

@sholderbach sholderbach merged commit 80220b7 into nushell:main Sep 28, 2023
19 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Sep 28, 2023

Darn, I forgot about gsudo, that I sometimes use on windows. Now that I think about it, maybe it also has a sudo version of the command?

@ZerdoX-x
Copy link
Contributor Author

I think we should in the future have a way to simply provide a list of commands or a signature that will take external commands and arguments in the trailing position.

Definitely. As Darren mentioned, it may be some other commands or just any command that wraps other binaries. I could not find a way to implement custom completion with current level of interfaces and/or documentation.

Do you want me to open an issue for this?

@fdncred
Copy link
Collaborator

fdncred commented Sep 30, 2023

Do you want me to open an issue for this?

I think so. Thanks!

@p00f
Copy link
Contributor

p00f commented Oct 17, 2023

@ZerdoX-x before this PR, I used to do it with

let doas_completer = {|spans: list<string>|
    do $env.config.completions.external.completer ($spans | skip 1)
}

# This completer will use carapace by default
let external_completer = {|spans|
    let expanded_alias = (scope aliases | where name == $spans.0 | get -i 0.expansion)
    let spans = if $expanded_alias != null  {
        $spans | skip 1 | prepend ($expanded_alias | split words)
    } else {
        $spans
    }

    match $spans.0 {
        doas => $doas_completer
        _ => $carapace_fish_completer
    } | do $in $spans
}
$env.config = {
    completions: {
        external: {
            enable: true
            completer: $external_completer
        }
    }
}

@p00f
Copy link
Contributor

p00f commented Oct 17, 2023

note that pkexec is another sudo-like command

@ZerdoX-x
Copy link
Contributor Author

before this PR, I used to do it with

I think anyways I'll open another issue since there are plenty of commands who accept another program as their first parameter. The most basic example here is which, which is not supported by nushell built-in completions. I am not sure this is user friendly, to make people write this in order to support which, exec and other commands :D

@p00f
Copy link
Contributor

p00f commented Oct 20, 2023

which just takes other programs, you don't have to provide completions for the other program's arguments. agree on exec

hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# Description

Fixes nushell#2047 but for the `doas` command the same way as in nushell#8094

# User-Facing Changes
No breaking changes. If people not using `doas`, no difference at all.

# Tests
I have not added any tests since its using same logic as for "sudo". I
guess if something would go wrong in this part, sudo tests will cover
it?

# Additional context
As a nushell user I could not find a way to implement custom completion
for a "sudo like command". Since I can see `sudo` being hardcoded in
sources, this is what I propose.

~~Also I have almost zero knowledge of rust and this is definitely not
the clean way yet~~

---------

Co-authored-by: Stefan Holderbach <sholderbach@users.noreply.github.com>
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.

Autocomplete does not work with sudo
4 participants