Fix multiple fish completion issues

[marckhouzam]

Fixes #1248
Fixes #1303
Fixes #1214
Fixes #1306

This PR fixes multiple bugs in the fish completion script.

Please ses each issue for details.

[marckhouzam]

Here is how to test this specific fix. An extensive test program is included at the bottom.

Test 1 - ShellCompDirectiveNoSpace not respected
Before the PR:

$ fish
$ ./testprog completion fish | source

$ ./testprog nospace compo<TAB>
# and
$ ./testprog --nospace compo<TAB>

will complete to:

$ ./testprog nospace compost<SPACE>
# and
$ ./testprog --nospace compost<SPACE>

the <SPACE> is wrong at the end of the completion because ValidArgsFunction requested ShellCompDirectiveNoSpace, as can be seen by:

$ ./testprog __complete nospace compo
complete	description1
compost	description2
:2
Completion ended with directive: ShellCompDirectiveNoSpace

Testing with this PR in the exact same way will not output the <SPACE> after the word compost.

Test 2 - ShellCompDirectiveNoSpace not properly handling descriptions

The nospace logic for fish does not handle the case when there is a description. To achieve "nospace" the script adds a second completion with an extra "." to prevent the shell from adding a space. However if there is a description, the extra "." is added to the description instead of the completion, and the nospace directive does not work.

Before the PR:

$ fish
$ ./testprog completion fish | source

$ ./testprog nospace first compo<TAB>

will complete to:

$ ./testprog nospace first compo<SPACE>

the <SPACE> is wrong at the end of the completion because ValidArgsFunction requested ShellCompDirectiveNoSpace, as can be seen by:

$ ./testprog __complete nospace compo
compost	description2
:2
Completion ended with directive: ShellCompDirectiveNoSpace

Notice this is slightly different than the previous case because a single completion is returned by the __complete command, but the <SPACE> is still added, this time because of the description.

Testing with this PR in the exact same way will not output the <SPACE> after the word compost.

Test 3 - File completion not always done when requested

Before the PR:

$ fish
$ ./testprog completion fish | source
$ touch myfile

$ ./testprog withfile myfi<TAB>

will not perform file completion and will not show myfile although it should have.

Testing with this PR in the exact same way will trigger file completion and will show myfile as expected.

Extensive test program to execute the above tests, but also any regression tests:

package main

import (
	"fmt"
	"os"
	"strings"

	"github.com/spf13/cobra"
)

var completionNoDesc bool

var completionCmd = &cobra.Command{
	Use:   "completion [bashzsh|fish|powershell]",
	Short: "Generate completion script",
	Long: `To load completions:

Bash:

$ source <(testprog completion bash)

# To load completions for each session, execute once:
Linux:
  $ testprog completion bash > /etc/bash_completion.d/testprog
MacOS:
  $ testprog completion bash > /usr/local/etc/bash_completion.d/testprog

Zsh:

$ source <(testprog completion zsh)
$ compdef _testprog testprog

# To load completions for each session, execute once:
$ testprog completion zsh > "${fpath[1]}/_testprog"

Fish:

$ testprog completion fish | source

# To load completions for each session, execute once:
$ testprog completion fish > ~/.config/fish/completions/testprog.fish

PowerShell:

PS C:\> testprog completion powershell | Out-String | Invoke-Expression

To load completions for every new session, add the output of the above command
to your powershell profile.
`,
	DisableFlagsInUseLine: true,
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"bash\tBash completion", "zsh", "fish", "powershell"}, cobra.ShellCompDirectiveNoFileComp
	},
	Args: cobra.ExactValidArgs(1),
	Run: func(cmd *cobra.Command, args []string) {
		switch args[0] {
		case "bash":
			cmd.Root().GenBashCompletion(os.Stdout)
		case "zsh":
			if !completionNoDesc {
				cmd.Root().GenZshCompletion(os.Stdout)
			} else {
				cmd.Root().GenZshCompletionNoDesc(os.Stdout)
			}
		case "fish":
			cmd.Root().GenFishCompletion(os.Stdout, !completionNoDesc)
		case "powershell":
			cmd.Root().GenPowerShellCompletion(os.Stdout)
		}
	},
}

var rootCmd = &cobra.Command{
	Use: "testprog",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("rootCmd called")
	},
}

var vargsCmd = &cobra.Command{
	Use:        "vargs",
	Short:      "Cmd with ValidArgs AND a subcmd",
	ValidArgs:  []string{"one", "two", "three"},
	ArgAliases: []string{"un", "deux", "trois"},
	Args:       cobra.MinimumNArgs(1),
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("vargsCmd called")
	},
}

var childVargsCmd = &cobra.Command{
	Use:   "childVargs",
	Short: "Child command",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("childVargs called")
	},
}

var aliasCmd = &cobra.Command{
	Use:     "cmdWithAlias",
	Short:   "Command with aliases",
	Aliases: []string{"aliasCmd", "cmdAlias"},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("cmdWithAlias called")
	},
}

// A command with no short description
var nodescriptionCmd = &cobra.Command{
	Use: "nodesc",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("nodesc called")
	},
}

// File filtering for the first argument
// Replaces MarkZshCompPositionalArgumentFile(1, string{"*.log", "*.txt"})
var filterFileCmd = &cobra.Command{
	Use:   "filterFile",
	Short: "Only list file of type [*.log,*.txt]",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) == 0 {
			return []string{"log", "txt"}, cobra.ShellCompDirectiveFilterFileExt
		}
		// No more expected arguments, so turn off file completion
		return nil, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("filterFile called")
	},
}

// Turn off file completion for all arguments
var noFileCmd = &cobra.Command{
	Use:   "nofile",
	Short: "No file completion done",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return nil, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("nofile called")
	},
}

// Full file completion for arguments
var fullFileCmd = &cobra.Command{
	Use:   "fullFile [filename]",
	Short: "Full file completion",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("fullFile called")
	},
}

// Only allow directory name completion and only for the first argument
var dirOnlyCmd = &cobra.Command{
	Use:   "dirOnly [dirname]",
	Short: "Only list directories in completion",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) == 0 {
			return nil, cobra.ShellCompDirectiveFilterDirs
		}
		// No more expected arguments, so turn off file completion
		return nil, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("dirOnly called")
	},
}

// Only allow directory name from within a specified directory and only for the first argument
var subdirCmd = &cobra.Command{
	Use:   "subdir [sub-dirname]",
	Short: "Only list directories within themes/",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) == 0 {
			return []string{"themes"}, cobra.ShellCompDirectiveFilterDirs | cobra.ShellCompDirectiveNoFileComp
		}
		// No more expected arguments, so turn off file completion
		return nil, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("subdir called")
	},
}

// Test file completion when returning other completions
var compAndFileCmd = &cobra.Command{
	Use:   "compAndFile [sun|moon|<filename>]",
	Short: "Provide some completions and file completion when prefix does not match",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) != 0 {
			return []string{"nginx", "thanos"}, cobra.ShellCompDirectiveNoFileComp
		}
		var completions []string
		for _, comp := range []string{"install", "uninstall"} {
			completions = append(completions, comp)
		}
		return completions, cobra.ShellCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("compAndFile called")
	},
}

var colonCmd = &cobra.Command{
	Use:       "cmdColonArgs",
	Short:     "Command with colon args",
	ValidArgs: []string{"first:ONE", "first:SECOND"},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("colonCmd called")
	},
}

var longDescCmd = &cobra.Command{
	Use:       "cmdLongDesc12345678901234567890123456789012345678901234567890",
	Short:     `We have here a quite long description which should go over the length of the shell width.  What should we do with such a long description? zsh and fish handle it nicely, so we should prepare bash to handle it in the same fashion`,
	ValidArgs: []string{"first:ONE", "first:SECOND"},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("colonCmd called")
	},
}

var specialCharDescCmd = &cobra.Command{
	Use:   "specialChar",
	Short: "Description contains chars like ` and maybe others",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("specialChar cmd called")
	},
}

var noSpaceCmd = &cobra.Command{
	Use:   "nospace",
	Short: "followed by completion with noSpace, even for file completion",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		comps := []string{"complete\tdescription1", "compost\tdescription2"}
		if len(args) == 0 {
			// Allow file completion with ShellCompDirectiveNoSpace
			return comps, cobra.ShellCompDirectiveNoSpace
		}

		var finalComps []string
		for _, comp := range comps {
			if strings.HasPrefix(comp, toComplete) {
				finalComps = append(finalComps, comp)
			}
		}
		return finalComps, cobra.ShellCompDirectiveNoSpace
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("nospace cmd called")
	},
}

var sentenceCmd = &cobra.Command{
	Use:   "sentence",
	Short: "completion has spaces",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		// Allow file completion with ShellCompDirectiveNoSpace
		return []string{"comp is a sentence\tFirst comp", "and another sentence\tsecondComp"}, cobra.ShellCompDirectiveNoFileComp
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("sentence cmd called")
	},
}

var withfileCmd = &cobra.Command{
	Use:   "withfile",
	Short: "completion always returned and has file completion",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		// We do not consider the toComplete prefix and always return a completion
		return []string{"why"}, cobra.ShellCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("withfile cmd called")
	},
}

func setFlags() {
	rootCmd.Flags().String("theme", "", "theme to use (located in /themes/THEMENAME/)")
	rootCmd.Flags().SetAnnotation("theme", cobra.BashCompSubdirsInDir, []string{"themes"})

	rootCmd.Flags().String("theme2", "", "theme to use (located in /themes/THEMENAME/)")
	rootCmd.RegisterFlagCompletionFunc("theme2", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"themes"}, cobra.ShellCompDirectiveFilterDirs
	})

	rootCmd.Flags().String("customComp", "", "test custom comp for flags")
	rootCmd.RegisterFlagCompletionFunc("customComp", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"firstComp\tthe first value", "secondComp\tthe second value", "forthComp\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
	})

	rootCmd.Flags().StringP("file", "f", "", "list files")
	rootCmd.Flags().String("longdesc", "", "before newline\nafter newline")

	rootCmd.Flags().BoolP("bool", "b", false, "bool flag")
	rootCmd.Flags().BoolSliceP("bslice", "B", nil, "bool slice flag")

	rootCmd.PersistentFlags().StringP("persistent", "p", "", "persistent flag")

	// rootCmd.Flags().String("required", "", "required flag")
	// rootCmd.MarkFlagRequired("required")

	vargsCmd.Flags().StringP("nonpersistent", "n", "", "non persistent local flag")

	// If there is only a recommended value before we reach the equal sign or the colon, then the completion works as expected.
	// If the values differ afterwards, completion fails in bash, but works in fish and zsh
	rootCmd.Flags().String("equalSignWorks", "", "test custom comp with equal sign")
	rootCmd.RegisterFlagCompletionFunc("equalSignWorks", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"first=Comp\tthe first value", "second=Comp\tthe second value", "forth=Comp\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
	})

	rootCmd.Flags().String("equalSignWorksNot", "", "test custom comp with equal sign")
	rootCmd.RegisterFlagCompletionFunc("equalSignWorksNot", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"first=Comp1\tthe first value", "first=Comp2\tthe second value", "first=Comp3\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
	})

	rootCmd.Flags().String("colonWorks", "", "test custom comp with colon one value works")
	rootCmd.RegisterFlagCompletionFunc("colonWorks", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"first:ONE\tthe first value", "second\tthe second value", "forth\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
	})

	rootCmd.Flags().String("colonWorksNot", "", "test custom comp with colon doesnt work")
	rootCmd.RegisterFlagCompletionFunc("colonWorksNot", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"first:ONE\tthe first value", "first:SECOND\tthe second value", "second:Comp1\tthe forth value"}, cobra.ShellCompDirectiveNoFileComp
	})

	rootCmd.Flags().String("nospace", "", "test custom comp with nospace directive")
	rootCmd.RegisterFlagCompletionFunc("nospace", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return []string{"complete\tFirst comp", "compost\tsecondComp"}, cobra.ShellCompDirectiveNoSpace
	})

	rootCmd.Flags().StringArray("array", []string{"allo", "toi"}, "string array flag")
}

func main() {
	completionCmd.Flags().BoolVar(
		&completionNoDesc,
		"no-descriptions", false,
		"disable completion description for shells that support it")
	rootCmd.AddCommand(completionCmd)

	rootCmd.AddCommand(
		filterFileCmd,
		noFileCmd,
		fullFileCmd,
		dirOnlyCmd,
		subdirCmd,
		compAndFileCmd,
		nodescriptionCmd,
		vargsCmd,
		colonCmd,
		aliasCmd,
		longDescCmd,
		specialCharDescCmd,
		noSpaceCmd,
		sentenceCmd,
		withfileCmd,
	)

	vargsCmd.AddCommand(childVargsCmd)
	setFlags()
	rootCmd.Execute()
}

[github-actions]

This PR is being marked as stale due to a long period of inactivity

[marckhouzam]

Waiting for review

[Luap99]

Given that you are already touching this line can you change it to

Suggested change

	set args (string split -- " " (commandline -c))
	set args (string split -- " " (string trim (commandline -c)))

Please check #1303 for details. I already checked that both $argv or commandline -c have to be trimmed

[marckhouzam]

Thanks @Luap99 !
I will update with your findings.

[Luap99]

Actually it has to be:

Suggested change

	set args (string split -- " " (commandline -c))
	set args (string split -- " " (string trim -l (commandline -c)))

Only trim the left spaces as the right ones are required if complete an empty arg (e.g. command [TAB]).

[marckhouzam]

Done.
Thanks again!

[krobelus]

This line should probably be set args (commandline -opc).
If the token at the cursor is really required, (commandline -ct) can be appended.

[marckhouzam]

This PR now addresses 5 different fish completion issues.

We have Go tests for the completion logic in Cobra that is coded in Go. Those tests can be found in custom_completions_test.go. However, we don't have tests to exercise the actual completion script coded in shell script. This makes it more difficult to make changes to those shell scripts without worrying about causing some regression.

To try to address this concern, I have setup a regression-testing project for the Cobra completion scripts.
It is currently able to test the fish and the bash scripts. Specifically for fish, it has tests that fail for each issue this PR is fixing and that pass with this PR applied. This gives me good confidence in this PR.

If anyone is interested it should be simple to run those tests locally:
https://github.com/marckhouzam/cobra-completion-testing

[Luap99]

LGTM

[Luap99]

@marckhouzam CI is red. You have to rebase.

[krobelus]

Should be string trim -- -l (commandline -cp)

---

Often it's not necessary to pass the current token to fish.
Rather, one can just compute all possible tokens because fish will filter them anyway.
Sometimes it can be useful to do the filtering in the app (for example if it's not feasible to enumerate all possible tokens), so it's fine. (the testprog examples show that the current token is necessary to complete --flags)

[marckhouzam]

Should be string trim -- -l (commandline -cp)

I don't understand from the documentation what the -p flag does. Could you clarify?

Often it's not necessary to pass the current token to fish.
Rather, one can just compute all possible tokens because fish will filter them anyway.
Sometimes it can be useful to do the filtering in the app (for example if it's not feasible to enumerate all possible tokens), so it's fine. (the testprog examples show that the current token is necessary to complete --flags)

Right. So originally, I thought the program should do the filtering, but with experience, I now realize it is actually better to provide fish (and zsh) with all unfiltered completions so that the shell can do smarter matching than simply on prefix. For example, if the user types prog paw<TAB>, giving shell completions like bearpaw will allow it to still match even though it is not on the prefix.

As you saw in this PR however, I didn't find a way to find out if fish had found a smart match or not, so, when wanting to do file completion, I was forced to filter on prefix in the script itself, to know if there would be a match or not. This is because Cobra only wants to do file completion if there are no valid completions, so it can be consistent with bash and zsh. This was not ideal, so if you know of a way to figure out in the script if fish will match on a list of completion, that would be helpful.

[krobelus]

The -p is explained in #1249 (comment). Sorry for the wall of text 😸

fish does fuzzy substring matching if there is no prefix or exact match. This is currently not exposed in a scriptable way.

I wonder if it's acceptable to do file completions only if there are no results. Doesn't sound too good, so probably the prefix comparison is better (maybe make that substring matching instead of only prefix).

[marckhouzam]

Should be string trim -- -l (commandline -cp)

Aha, I get it now. Thanks. Yeah, that is important! I will add the -p.
I will also add a regression test for this case in https://github.com/marckhouzam/cobra-completion-testing

[marckhouzam]

I wonder if it's acceptable to do file completions only if there are no results.

You mean if the program returns no completions? This is what the script does right now (before this PR). But that requires that the program do the filtering on itself. This PR is (in part) trying to improve that situation. By doing the filtering in the fish script, it will allow the program to return all completions, which will then allow the fuzzy matching you mention. The fuzzy matching will only not work when file completion is requested or when nospace is requested, both of which are the less common cases.

Doesn't sound too good, so probably the prefix comparison is better (maybe make that substring matching instead of only prefix).

As long as we use an algorithm that is stricter than what fish does, we could indeed improve compared to filtering on prefix. Do you know where I could see the algorithm used by fish? Or else I'll just play around and try to see if any pattern match seems to work.

[krobelus]

Just to clarify for myself; the common cases are

• no files
• only files because the program returns no completions

in those cases fish already does all the filtering. But if we're in between - the program returns some completions but they may not match the token - we want to filter in the completion script. That sounds great.

I agree that this is probably not very common. Most programs should take either custom tokens (like subcommands) or files, not both. Git is a notable exception, but Git completion computes custom file paths based on git ls-files.

I'd just ignore nospace for fish for now until, it behaves the right way most of the time already.
I can't think of a good solution right now, it needs support from fish.

Do you know where I could see the algorithm used by fish?

The two most permissive filters are here. I think they can be implemented like this:

$ function fishy_match -a needle haystack
      if string match -irq -- (string escape --style=regex -- $needle) $haystack
          echo substr match
      end
      if string match -rq -- (string split -- '' $needle | string escape --style=regex | string join '.*?') $haystack
          echo subseq match
      end
  end
$ fishy_match String 'case-insensitive substring match'
substr match
$ fishy_match foo follow
subseq match

I use -r and regex escaping because the needle might contain globs, which would be interpreted by string match without -r.

[marckhouzam]

Most programs should take either custom tokens (like subcommands) or files, not both.

There are some cases where both are valid. Helm for example can accept the reference to a remote chart, or the local path to a chart. What I've done in helm is have the program return the list of files as completions to do both file completion and custom token completion.

I think it is important to keep all shells behaving the same for Cobra because the program returns a list of completions to be used by all shells; we don't want to make the program have to behave differently for each shell.

But we could, as a separate effort, support doing file completion even if there are custom token completion matches. This was not Cobra's previous behaviour, so I had never considered it myself, but I believe it could be done for all shells. Maybe a future effort...

I'd just ignore nospace for fish for now until, it behaves the right way most of the time already.
I can't think of a good solution right now, it needs support from fish.

Well, our current solution is good until fish 3.2. And then, when fish shows the choices including the fake one with a trailing ., it may be confusing, but I think it may be not much worse than letting the shell add and unwanted space.

DONE - I'll turn off adding the extra . when fish handles the case itself following the characters you mentioned.

PENDING - And I'll also adapt the filtering based on the algos you've provided.

I feel that after all that, we'll have something really solid for fish completion.

[krobelus]

What I've done in helm is have the program return the list of files as completions to do both file completion and custom token completion.

Ideally we could tell the shell that we also want file completons, because it can see through variables to tokens like $PWD/. Something for the future I guess. The implementation would be a bit hacky:
essentially, you'd need two calls to complete -c, one with -f and one without.
Each of them has a condition (the -n part). If the condition for the one with file completions is true, then fish will also provide file completions (in addition to the provided custom completions).

we don't want to make the program have to behave differently for each shell.

In terms of what completions to offer, definitely. Of course shells differ in how they display completions.
By default, fish sorts completions, but you can add the --keep-order flag to the complete -c call, then the program is in control of the ordering.

I feel that after all that, we'll have something really solid for fish completion.

Yeah, latest changes look great! For some programs it's hard to compete with manually written completions, but it should work great for conventional programs.

[krobelus]

This is clever, and works reliably as far as I can tell.
However, the upcoming fish 3.2.0 will always show the pager on ambiguous completions (fish-shell/fish-shell#6924). This would cause some confusion.

So I'm not sure if this an overall improvement. Who are the users of this option?
Fish already gets this right a lot of times by hardcoding the character set @=/:., - if a completion ends in one of these characters, then no space will be inserted.

[marckhouzam]

This is clever, and works reliably as far as I can tell.
However, the upcoming fish 3.2.0 will always show the pager on ambiguous completions (fish-shell/fish-shell#6924). This would cause some confusion.

Ouch, that will indeed cause confusion for the "nospace" case.

So I'm not sure if this an overall improvement. Who are the users of this option?

For example, Helm uses this option when completing a chart's full name which is comprised of <repository>/<chartname>; the completion first provides all repository names in the form <repository>/, and once the slash is present on the command-line, the completion will add the possible chart names, but there should be no space between the two. This is much more efficient for helm than having to provide all the full <repository>/<chartname> from the very start.

kubectl cp <pod>:<file> could also use this option to avoid adding a space after the :.

Fish already gets this right a lot of times by hardcoding the character set @=/:., - if a completion ends in one of these characters, then no space will be inserted.

Cool! I was not aware of this. This would address both cases I mentioned. I tested it and it works as you describe.
But I don't know if there are other cases that wouldn't end with @=/:.,. At a minimum, we could turn off this hack for if the completion ended with @=/:.,. I'll have to think what to do for the other (possible) cases for fish 3.2. Let me know if you have any suggestions.

[krobelus]

One could sidestep the completion engine and directly insert the unique completion if nospace is set

$ function sneaky-complete
      commandline --replace -t nospace
  end
$ complete -e sneaky-complete; complete -c sneaky-complete -a '(sneaky-complete)' -f

According to my tests this works (although it rings the terminal bell, because fish thinks there are no completions). However, I'd advise against it. You are not meant to modifiy the commandline during the computation of a completion. That can break other completions, though I didn't find an example here.

fish has not added an option to control the add-a-space behavior yet because AFAIK no real use case has been brought up that doesn't work with the hardcoded list.

I fear that the cost of adding a hack to support this outweighs the benefits.
I think we should drop this until we see a real need. Then an option can be added to fish, avoiding the need for a hacky workaround.

[Luap99]

I took a look how I use the ShellCompDirectiveNoSpace option in podman. Almost all cases ends with one of =:, but not all. I found one case where I set this when it ends with a -. https://github.com/containers/podman/blob/479fc226044b745ecaafc2d5fa925afe6ca06de0/cmd/podman/common/completion.go#L603-L608
Even more importantly there is a case where I set this together with ShellCompDirectiveDefault (path completion). This case does not work with fish currently. It works for bash with @marckhouzam bashV2 PR.

[krobelus]

Thanks for investigating. I guess we should just add - to the set of separators in fish.

Regarding the nospace+path, is this what you mean:

// cobra-completion-testing/testprog/testprog.go
var noSpaceCmdPrefix = &cobra.Command{
	Use:   "nospace",
	Short: "Directive: no space",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		return nil, cobra.ShellCompDirectiveNoSpace
	},
	Run: func(cmd *cobra.Command, args []string) {},
}

testprog/bin/testprog prefix nospace <TAB>

this gives me files in both fish and bash (I've merged this branch and the bash v2 one).
If I return some completions instead of nil, then I only get those completions in both shells (as expected).

You probably mean something else?

[Luap99]

Honestly I don't think adding - make much sense. This might solve my specific problem but doesn't solve the problem here.
cobra allows me as developer to set nospace wherever I want, not only on those characters. I don't see a reason why fish should limit my options as a completion script developer.

[marckhouzam]

Even more importantly there is a case where I set this together with ShellCompDirectiveDefault (path completion).
Bash does not add a space after the completed path. Fish does.

@Luap99 I never planned for this case. And now that you mention it, I realize it also does not work for zsh. It is an easy fix though. I will add it to #1213. And I see that you made it work for powershell 🎉 . I don't believe there is anything we can do for fish though.

Out of curiosity, what kind of situation would not want a space after file completion?

[marckhouzam]

Turns out preventing a space after file completion for zsh also prevents the trailing / from being added after a directory. At least in the way I've found to prevent a space. I think a missing / is a bigger problem than the extra space, so I'll leave it alone.

[marckhouzam]

To get back to the 'nospace' directive for fish, the latest version of this PR keeps the "hack" that adds an extra fake completion but only when the completion does not end with @=/:.,. I think this is the best we can do at this time.

[Luap99]

Out of curiosity, what kind of situation would not want a space after file completion?

Simple answer podman/docker run --volume. This flag accepts a volume name or a path followed by a colon and again a path. This is my current autocompletion function:

func AutocompleteVolumeFlag(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
	volumes, _ := getVolumes(cmd, toComplete)
	directive := cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveDefault
	if strings.Contains(toComplete, ":") {
		// add space after second path
		directive = cobra.ShellCompDirectiveDefault
	}
	return volumes, directive
}

After the first path I do not want the space added. The user has to type a colon here. After the colon you get only path completion and then I a space should be added.

Turns out preventing a space after file completion for zsh also prevents the trailing / from being added after a directory. At least in the way I've found to prevent a space. I think a missing / is a bigger problem than the extra space, so I'll leave it alone.

I am totally fine if it does not work. It is just a small corner case.