Fish completion (including support for Go custom completion)

[marckhouzam]

This is an implementation of Fish completion based on the Custom Go Completions of PR #1035

The PR takes a different approach to completion. Instead of coding the entire completion in shell script, it delegates to the Go program the responsibility of determining the completion choices. This makes it simpler to implement completion support, as Cobra provides all the necessary info to easily determine completions.

It automatically supports custom completions once they have been migrated to the Custom Go Completion method of PR #1035.

And it supports automatically adding the "usage" string for command and flag completions to describe these completions. This is something bash completion doesn't have.

I haven't yet tried to support the other annotations BashCompFilenameExt, BashCompOneRequiredFlag or BashCompSubdirsInDir.

To easily try this out I have adapted kubectl in my repo to use this PR. Bear in mind that I haven't migrated the custom completions for kubectl (e.g., kubectl get <TAB>) to use the new Custom Go Completions of #1035, so custom completions will not yet work with this PR. The kubectl that works with basic completions is here: https://github.com/VilledeMontreal/kubernetes/tree/feat/fishCompletion

My experience with Fish is limited, so it would be good for others to try this out and provide feedback.

[marckhouzam]

I will get this one ready for review ASAP.

[marckhouzam]

Ok, I just pushed the rebased feature.

However, I'd like to take the time to make a small improvement I recently thought of. Both Fish shell and Zsh allow for completions to be associated with a description shown to the user. This PR currently provides the description for sub-commands (the Command.Short string) and flags (the Flag.Usage string) , just like Cobra's current zsh completion does. With a couple of minor changes I believe we can also allow ValidArgs and ValidArgsFunction to support providing descriptions for their completions.

It won't change the feature much, so it is fine to start playing with it already.

[marckhouzam]

Here is how to test this feature.

Slightly modified test program that can generate the Fish completion script:

package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	ValidArgs: []string{"replica\tanother", "rs\tdescription for rs", "ds"},
	Args:      cobra.ExactArgs(1),
	Use:       "testprog1",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("rootCmd called")
	},
}

var childCmd = &cobra.Command{
	Use:   "pod",
	Short: "Start the pod",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.BashCompDirective) {
		return []string{"these", "are\tverb", "custom\tadjective", "completions\tnoun, plural"}, cobra.BashCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("child called")
	},
}

var childCmd2 = &cobra.Command{
	Use:   "svc",
	Short: "Launch the service",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.BashCompDirective) {
		return []string{"second", "level"}, cobra.BashCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("child called")
	},
}

var completionCmd = &cobra.Command{
	Use:                   "completion [bash|zsh|fish] [--no-descriptions]",
	Short:                 "Generate completion script",
	DisableFlagsInUseLine: true,
	ValidArgs:             []string{"bash\t", "zsh", "fish"},
	Args:                  cobra.ExactArgs(1),
	Run: func(cmd *cobra.Command, args []string) {
		switch args[0] {
		case "bash":
			cmd.Root().GenBashCompletion(os.Stdout)
			break
		case "zsh":
			cmd.Root().GenZshCompletion(os.Stdout)
			break
		case "fish":
			cmd.Root().GenFishCompletion(os.Stdout, !completionNoDesc)
			break
		}
	},
}

var (
	completionNoDesc bool
	testFlag         string
)

func main() {
	completionCmd.PersistentFlags().BoolVar(
		&completionNoDesc,
		"no-descriptions", false,
		"disable completion description for shells that support it")
	completionCmd.PersistentFlags().StringVar(
		&testFlag,
		"test-flag", "",
		"just to test")
	rootCmd.AddCommand(childCmd)
	rootCmd.AddCommand(completionCmd)
	childCmd.AddCommand(childCmd2)
	rootCmd.Execute()
}

Some tests step (the below output in the comments hasn't been updated to reflect the latest version of the above test program):

GO111MODULE=on go build -o testprog1 main.go
fish --version   # fish, version 3.1.0
fish
./testprog1 completion fish | source

./testprog1 <TAB>
# Should show: completion  (Generate completion script)  ds  pod  (Start the pod)  rs
# The descriptions are shown in parentheses.  Notice that the lack of description for ValidArgs
# is kind of annoying in the output.  That is what I want to improve.
# Notice also that both sub-commands and ValidArgs are being shown (based on fixing #1076)

./testprog1 pod <TAB>
# Should show: are  completions  custom  svc  (Launch the service)  these

./testprog1 completion --<TAB>
# Should show: --no-descriptions  (disable completion description for shells that support it)  --test-flag  (just to test)  --test-flag=  (just to test)

# To disable descriptions (program can choose to make this an option for users)
./testprog1 completion fish --no-descriptions | source

./testprog1 <TAB>
# Should now show: completion ds  pod rs

./testprog1 pod <TAB>
# Should now show: are  completions  custom  svc  these

./testprog1 completion --<TAB>
# Should now show: --no-descriptions  --test-flag --test-flag=  

# To debug the ValidArgsFunction code
./testprog1 __complete com
# After pressing <enter> should show:
completion
:4
Completion ended with directive: BashCompDirectiveNoFileComp

[jpmcb]

Wow, great work on this! Went through your testing script and everything works.

I also tested that the source file could be loaded during fish initialization:

fish>  ./testprog1 completion fish --no-descriptions > ~/.config/fish/conf.d/testprog.fish
fish>  exit

# load a new fish shell
fish> ./testprog <TAB>
# shows: completion ds  pod rs

Do we want to recommend in fish_completions.md users place the custom completion script in their ~./config/fish/conf.d folder or simply source it as you had?

If not, +lgtm

[marckhouzam]

Alright, this is ready for review.

I've made it possible to add a description text in ValidArgs like so:

&cobra.Command{
        ...
	ValidArgs: []string{"bash\tCompletion for bash", "fish\tCompletion for fish"},
        ...
}

and in a similar fashion in the output of ValidArgsFunction. It will work for Fish completion and will be silently ignored for Bash completion (bash does not support descriptions).

However, I expect that Cobra's Zsh completion requires some adjustments for this to be handled properly. But I don't yet understand the details of the Zsh completion done by Cobra. Therefore, I simply haven't documented the concept of adding descriptions to ValidArgs or ValidArgsFunction just yet. But for programs like helm and kubectl that don't use Cobra's Zsh completion, it will be possible to use right away.

[marckhouzam]

Wow, great work on this! Went through your testing script and everything works.
I also tested that the source file could be loaded during fish initialization

Thanks @jpmcb! I actually had not tested for fish initialization, so it is great that you did and confirm it works.

Do we want to recommend in fish_completions.md users place the custom completion script in their ~./config/fish/conf.d folder or simply source it as you had?

We could mention both in the readme, but in the end it will be up to the program using Cobra to make the final recommendation. Is one better than the other in the context of the Fish shell?

Also, let me ask about a point I haven't quite figured out for Fish. Notice in the script the lines:

# Remove any pre-existing completions for the program since we will be handling all of them
# TODO this cleanup is not sufficient.  Fish completions are only loaded once the user triggers
# them, so the below deletion will not work as it is run too early.  What else can we do?
complete -c testprog1 -e

The TODO is about completions already provided by Fish. For example, Fish provides a completion script for helm (see https://github.com/fish-shell/fish-shell/blob/master/share/completions/helm.fish). The problem is that this script is out of date and provides invalid completions for the latest version of Helm. I'm looking for a way to force the loading of the original Fish completions for helm so that once we load the new script of this PR, it will delete the old ones. I haven't found the solution yet...

[marckhouzam]

Oh, I forgot to mention that I updated the test program in the comment above to include extra descriptions.

[jpmcb]

# TODO this cleanup is not sufficient.  Fish completions are only loaded once the user triggers
# them, so the below deletion will not work as it is run too early.  What else can we do?

First, I do believe the provided complete line is sufficient for removing completions at the time of being run. From man complete:

When  erasing  completions,  it is possible to either erase all completions 
for a specific command by specifying complete -c COMMAND -e, or by 
specifying a specific completion option to delete by specifying either a long, 
short or old style option.

So, I believe that this line

complete -c %[1]s -e

is correct as it will erase all existing completions for the specified command at the time it is run or sourced. I've tested this out:

# set a list of some junk completions
fish> complete -c testprog1 -a 'bad completions`
fish> ./testprog1 <TAB>
# shows up with both 'bad' and 'completions'

# run and source the completion script which deletes existing completions
fish> ./testprog1 completion fish | source
fish> ./testprog1 <TAB>
# no longer see 'bad' or 'completions' but instead, the correct set from the go code.
# previous set completions are gone

This will at least remove any existing completions at that time of sourcing.

The problem is that this script is out of date and provides invalid completions for the latest version of Helm. I'm looking for a way to force the loading of the original Fish completions for helm so that once we load the new script of this PR, it will delete the old ones.

Good question. After some digging, I think this may actually be a symptom of fish's completion initialization ordering which appears to be slightly different from fish's shell initialization ordering. Placing this command:

complete -c helm -e

in a user defined config file will not remove the default completions as these are initialized separately from shell initialization.

But, looks like fish does provide a way to define some user completions. User set completions will take the highest precedence followed by "third party" software over default fish completions:

By default, Fish searches the following for completions, using the first available file that it finds:

• A directory for end-users to keep their own completions, usually ~/.config/fish/completions (controlled by the XDG_CONFIG_HOME environment variable);
  ...
• Directories for third-party software vendors to ship their own completions for their software. Fish searches the directories in the XDG_DATA_DIRS environment variable for a fish/vendor_completions.d directory; if this variable is not defined, the default is usually to search /usr/share/fish/vendor_completions.d and /usr/local/share/fish/vendor_completions.d;
• The completions shipped with fish, usually installed in /usr/share/fish/completions;

I've confirmed the default helm script you linked to is located at $__fish_data_dir/completions/helm.fish and is being loaded on my system as I have no other helm.fish files loaded in the mentioned locations.

It appears I was slightly wrong in my first review as the completion should have been:

fish>  ./testprog1 completion fish --no-descriptions > ~/.config/fish/completions/testprog.fish
fish>  exit

# load a new fish shell
fish> ./testprog <TAB>
# shows: completion ds  pod rs

instead of being placed in the user's conf.d folder. I've confirmed the above also works.

So, I believe that if an application developer wishes to have their completions supersede those bundled with default fish, they should recommend the user place the completion script in ether ~/.config/fish/completions/helm.fish or within their own vendor application defined configuration folder /usr/share/fish/vendor_completions.d/helm.fish.

Note that the name of the file must match the command name:

A completion file must have a filename consisting of the name of the command to complete and the suffix '.fish'.

I've tested this with helm and it appears to work:

fish> echo "complete -c helm -l debug" > ~/.config/fish/completions/helm.fish
fish> exit

# start new terminal
fish> helm -<TAB>
# auto completes with "debug" flag. No other options provided.

Maybe even on install, the application could automatically place it's completion script into the vendor directory at /usr/share/fish/vendor_completions.d/some-program.fish.

Regarding the changes since my last review, I've tested again with the program you provided and looks good! 👍

Apologies at the length of this review, let me know your thoughts @marckhouzam

[marckhouzam]

I rebased this PR to the master branch after the BashCompDirective to ShellCompDirective change.

[marckhouzam]

Great investigation @jpmcb on the whole loading completions. I have also test following your recommendation and you are correct that adding the completion script to ~/.config/fish/completions/helm.fish does take precedence over the helm completion script shipped with the Fish shell.

Maybe even on install, the application could automatically place it's completion script into the vendor directory at /usr/share/fish/vendor_completions.d/some-program.fish.

This is a good idea. For example, the Homebrew recipe install bash and zsh completion script for Helm; it would eventually also install the Fish completion as well.

But for the people that prefer to source the completion script, there would not be a good way to erase the completions shipped with Fish itself. I think the thing to do would be to contribute to the Fish project and use the Helm-generated completion as the default fish completion. Something to think about once all this is in.

[marckhouzam]

@jpmcb I notice you always use the --no-descriptions flag. Is there a reason for that? I'm looking for confirmation that there is value in providing this option to users.

[jpmcb]

No reason beyond it being the most recent command in my history and creating a little less noise. I do think it has value and that it's useful for people who don't want the long descriptions. I say leave it in!

@jharshman I think this is ready to go!

[marckhouzam]

Thank you @jharshman for your reviews!