-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Completion support for Nushell #1857
base: main
Are you sure you want to change the base?
Conversation
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Hey @ayax79 this exciting! I have never used I see that this PR generates a script containing the completions. However, this is no longer the approach used by Cobra. Instead, the program, through Cobra, provides the completions itself through the hidden This approach means that we have centralized all the logic to generate completions and coded it in Go (in Cobra's Following this approach would be your first step. You can look at the completion scripts for any of the other shells to see how this is done. These scripts are in files named for the shell they support. Eventually, the nushell script should also support the ShellCompDirectives, which you will notice in the other scripts. Feel free to ask for more info if needed. |
Hey @marckhouzam, Thanks for looking at this and thanks for the feedback! Nushell completions quite a bit differently than completions in other shells I have used (zsh). nushell_completions.go doesn't generate a script for the completions, it generates something more akin to a header file. each "export extern " maps to an external, non-nushell command. An example would be For example, see the minikube-completions.nu I generated. You can load it into nushell via (added .txt to get it to upload): I'll dig into the new model, but I would love any implementation advice. |
We had a similar situation with Fish where normal completions are a long list of choices. So in our case we have one generic completion entry which calls a function that calls The nushell doc talks about custom completions which seem like an avenue to investigate. For example it shows:
which calls |
An important advantage of Cobra's approach is that using the |
@rsteube the author of (carapace)[https://github.com/rsteube/carapace] chimed in with some good feedback on the nushell discord that I will include too. |
I have played around with a few options and haven't found anything that works well utilizing __command. This works for only the first level for commands and breaks flags:
To access a flag you have to bypass nushell via:
By adding another level:
This overrides the completion list and removes the description (unless it is manually added as comment). It also breaks any flags. The other option is to utilize the external completer support. This however only provides an option for one global fallback completer. This how carapace-bin works with nushell. I'll poke around more, but it seems like I am may be running out of options. |
Thanks for continuing the investigation @ayax79. In the above comment you typed It is also important to pass the current command-line arguments that the user typed. So, if the user typed Finally, if the last character on the command-line before the user pressed TAB is a space, you must pass an extra empty argument. So, if the user typed You can find documentation about all that here: https://github.com/spf13/cobra/blob/main/shell_completions.md#debugging |
Yeah you'll definitely have no success with the let external_completer = {|spans|
{
$spans.0: { } # default
kubectl: { kubectl __complete ($spans | into df | slice 1 1000) | handleOutput }
minikube: { minikube __complete ($spans | into df | slice 1 1000) | handleOutput }
} | get $spans.0 | each {|it| do $it}
} |
I finally got some more time to work on this. I also spent some time digging into the cobra completions and the nushell external completions code . This seems to work, although it needs some more testing. There might be some weirdness around mixing flags and commands:
As @rsteube mentioned, registration will be a little bit odd. I think that can be documented in commends for the completion generation though. It will require adding the that code block to the nushell config file ($nu.config-path) and updating config.external_completer entry:
One nice thing is that for other cobra commands, only the external_completer block will need to be updated. @marckhouzam, if this approach looks good, I'll update nushell_completer.go with it. |
Thanks @ayax79. I'm assuming that each tool,
The problem is that if I have multiple tools using this, only the last sourced script will work as each one sets the same After that, we need to figure out a way to have the one
manage to include all tools that have cobra completions. |
If we took this approach, it would still be possible to wrap the cobra completer and chain completers together -- it would be up to the user to figure how they wanted to handle it. The configuration itself isn't changeable once the shell is loaded. Nushell works much more like a compiled language. Dynamic loading of resources and execution isn't really possible. The two phase execution approach is part of the reason why nushell is so fast, but it does create some awkwardness. You can see how I got around this, by creating a subshell to execute the command I assembled:
What I really don't like about this approach is that nushell's environment has to load to execute the command (I think, need to verify). It might be feasible to do something kind of like execute completions from a directory, but it could get expensive:
Using something like carapace-bin is the best use of external_completers, because carpace itself is plugable. |
Not knowing anything about nushell, I'm having trouble following the subtleties. If we can reach a solution where any one tool can generate some nushell completion script and the user could add something to their configuration file to setup any number of such scripts, we would have something sufficient. |
I simplified the nushell external configurator to this:
The process would be:
There will be no need to change the configuration for any subsequent cobra based app. The above configuration will work with any cobra based app without any further change. |
Wow! After sourcing the script in the latest comment and then doing
I was able to do completion for helm and kubectl! One question with this approach: are we going to break completion for any non-cobra program that uses the external_completer? @ayax79 Could you update the PR with the new script and then I can start testing more thoroughly? One thing to investigate is that flag value completion does not seem to work:
We should be getting a list of namespaces. But this is a great start! |
The external completer is a fallback. Completions using externs will load first. If someone wants to use more than one external completer they could write a completer that wraps two:
I'll poke around with the flag completions. It should work the same as anything else. |
There was a bug in the line parsing causing it to drop entries that weren't exactly {value}\t{description}. Anything without a description was getting dropped. I changed to parse via regex. The only thing that might get dropped with this one is values (commands/flags/options) that have particularly exotic characters, I am matching [\w-.:+]. This is the corrected version:
I will have an updated pull request shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. Here are some things I noticed:
- Could you add
nushell
to the defaultcompletion
command? For example: https://github.com/spf13/cobra/blob/main/completions.go#L782-L785 - The
=
form of flags does not quite work. When doingkubectl -n=<tab>
, the-n=
part is removed - Probably linked to the previous point, doing
kubectl --namespace=<tab>
then selecting a completion choice, and then pressing backpace actually crashes the shell - When a completion is accepted a space should be added after it. For example
kubectl comple<tab>
should becomekubectl completion
(with a space aftercompletion
) ( except if theShellCompDirectiveNoSpace
is specified) - Would we be able to support the ShellComp directives? Or maybe a subset of them?
Lines 54 to 78 in 7bb1440
const ( // ShellCompDirectiveError indicates an error occurred and completions should be ignored. ShellCompDirectiveError ShellCompDirective = 1 << iota // ShellCompDirectiveNoSpace indicates that the shell should not add a space // after the completion even if there is a single completion provided. ShellCompDirectiveNoSpace // ShellCompDirectiveNoFileComp indicates that the shell should not provide // file completion even when no completion is provided. ShellCompDirectiveNoFileComp // ShellCompDirectiveFilterFileExt indicates that the provided completions // should be used as file extension filters. // For flags, using Command.MarkFlagFilename() and Command.MarkPersistentFlagFilename() // is a shortcut to using this directive explicitly. The BashCompFilenameExt // annotation can also be used to obtain the same behavior for flags. ShellCompDirectiveFilterFileExt // ShellCompDirectiveFilterDirs indicates that only directory names should // be provided in file completion. To request directory names within another // directory, the returned completions should specify the directory within // which to search. The BashCompSubdirsInDir annotation can be used to // obtain the same behavior but only for flags. ShellCompDirectiveFilterDirs - File completion does not seem to work: if there are no completions choices, the shell should suggests file names (except if the
ShellCompDirectiveNoFileComp
is specified)
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
1 similar comment
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
I just found a nasty bug. When running things vim, it locks up because vim interprets |
I think I am going to have to put a whitelist in for commands to execute the completer for:
I don't think it will be too onerous. The workflow will be: If you haven't setup cobra_completer
If you have setup cobra_completer
|
For other shells we don't use a "global" completer but instead have to register each program individually. So if kubectl generates the completion script, it also register If nushell completions don't work like that then your allow-list idea sounds like the right approach. I wonder if we could automate adding a program name to the list in the script itself? |
@rsteube if you have any guidance for us don't hesitate to chime in and correct us. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
We could certainly automate adding a line like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things are moving very nicely @ayax79. You probably didn't know what you were getting into when you first posted the PR 😄 but with a little more effort, we should get this in.
-
I now get a space after a single completion and the NoFileComp directive is respected. Nice work!
-
Is it normal the completion does not work if I use a path along with the program?
For example:kubectl [tab]
works but not~/bin/kubectl [tab]
-
Aliases work, nice! But...
alias k = kubectl
k [tab]
works, but most probably related to the previous point
alias k = ~/bin/kubectl
k [tab]
doesn't work
-
The = form of flags is starting to work but not completely.
For instancekubectl -n=[tab]
does work, but notkubectl -n=de[tab]
-
I'm getting a weird behaviour where no records are found when I think they should:
$ kubectl get ns
NAME STATUS AGE
default Active 23d
kube-system Active 23d
kube-public Active 23d
kube-node-lease Active 23d
$ kubectl get namespaces k[tab]
NO RECORDS FOUND
$ kubectl get namespaces ku[tab]
kube-system
kube-public
kube-node-lease
- New (advanced) requirement that I forgot about: the __complete command may
return completions that don't match the last argument the user typed.
For example:
$ helm __complete repo remove hello
test1 https://charts.bitnami.com/bitnami
test2 https://charts.bitnami.com/bitnami
test3 https://charts.bitnami.com/bitnami
bitnami https://charts.bitnami.com/bitnami
:4
Completion ended with directive: ShellCompDirectiveNoFileComp
Notice that although the last argument is "hello", the list of completions
does not filter on "hello". This case should be supported by Cobra because
it allows for shells that support it to do fuzzy matching. For example
with zsh
and fish
if I do helm repo remove nami[tab]
the shell will
match nami
with bitnami
.
This however is causing problems with the current nushell script. So:
$ helm repo remove bitn[tab]
test1
test2
test3
bitnami
Notice that nushell completion still suggests all completions and does not
complete the bitn
argument to bitnami
.
Therefore it seems the nushell completion script will need to do the filtering by
itself to get rid of all completions that don't match what the user has typed.
The cool thing is that I tested and it seems nushell will support fuzzy matching,
which will be richer than just matching on prefix. For example, nushell will allow:
$ helm repo remove nami[tab]
$ helm repo remove bitnami
The filtering rules in the script should be as follows:
1- try to match on prefix. If any completions start with what the user has typed,
then only those completions should be kept.
2- if no completion choices match on prefix, then we should look for any completions
that contain what the user typed
For example if the list of completions obtained is:
test
mytest
bitnami
origami
if the last argument typed is te[tab]
then we should only keep the completion
test
because it starts with te
(and remove the other 3 completions).
but if the user typed ami[tab]
, since nothing starts with ami
we match both
bitnami
and origami
which both contain ami
.
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
@marckhouzam I believe I have addressed all the feedback. Can you give this another look? |
@ayax79 I think this bug is still present. Maybe we have to tell the user to use |
If we have to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're getting so close I can taste it!
1- Is there a way to add debug statements to the nushell completion cobra_completer
. I want to make sure I can debug this in case there is a bug reported.
I was thinking we could add a function like this, maybe? But I'm having trouble calling it in the completer.
def debug [message] {
let file = $env.BASH_COMP_DEBUG_FILE?
if (not ($file == "")) {
echo $"($message)\n" | save $file --append
}
}
2- I'm hitting different bugs but as long as they are minor, I was thinking we can commit this in "beta level" and keep working on fixing it. However we need to fix the vim <tab>
hanging. In the mean time, I'll post a small program and give examples of the small issues I'm noticing.
3- Is there no way in nushell to interrupt the shell completion? I was testing with tanzu
and in some cases the tanzu __complete
command will trigger an interactive login and will wait there until I login. If I don't login, it will hand there forever. In zsh
I can press ^C to interrupt the ongoing completion.
"os" | ||
) | ||
|
||
func (c *Command) GenNushellCompletion(w io.Writer, includeDesc bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The includeDesc
variable is not used.
What you need to do is simple, if includeDesc == false
you want to use __completeNoDesc
in the script, instead of __complete
.
If we have a single cobra_completer
this won't really work as it will affect every program.
Let's see where we end up with having different completers and revisit at that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could allow this to be set via an environment variable? Maybe something like this:
$env.COBRA_NO_DESC_COMMANDS = [kubectl minikube]
This is pretty much what I do when debugging.
Sounds good.
I don't think so currently. I can investigate if it is possible to handle this in nushell core. The currently external completer stuff is pretty limited. You can define a block that is handled a list of spans and you can either return a list of records in the format of {value: "foo", description: "description of foo"} or null to no op. @fdncred, to you have any thoughts? |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
I'm not sure. I haven't worked with the completions much. I think the external completer is built in nushell but uses some reedline primitives. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
@marckhouzam, I added a logging function. |
Should we put a timeout when running the |
There isn't currently a way to handle timeouts on commands. Probably the best approach is to try to make the external completer support respect ^C. |
This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Completion support for Nushell. Besides the tests included, I have also tested against minikube.