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

Complete subcommands when TraverseChildren is set #1171

Merged
merged 2 commits into from
Sep 9, 2020

Conversation

Luap99
Copy link
Contributor

@Luap99 Luap99 commented Jul 17, 2020

The current custom completion logic does not complete
subcommands when a local flag is set. This is good unless
TraverseChildren is set to true where local flags
can be set on parent commands.

This commit allows subcommands to be completed only
if TraverseChildren is true.

Closes #1170
Closes #1172

@Luap99 Luap99 force-pushed the compSubCmd-TraverseChildren branch from 1da7a4f to 1d97c85 Compare July 18, 2020 14:48
Copy link
Collaborator

@marckhouzam marckhouzam 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 this. The TraverseChildren case had been completely missed.

I have a couple of suggested changes.

custom_completions.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions.go Outdated Show resolved Hide resolved
custom_completions.go Outdated Show resolved Hide resolved
@marckhouzam
Copy link
Collaborator

Also @Luap99, would you mind writing an issue to mention that TraverseChildren is not being considered for bash completion?

@marckhouzam
Copy link
Collaborator

@Luap99 I forgot to ask you to update your commit message, it should have a short title before going into the details.

@Luap99 Luap99 force-pushed the compSubCmd-TraverseChildren branch 2 times, most recently from b856c0c to 1def525 Compare July 19, 2020 14:35
@Luap99
Copy link
Contributor Author

Luap99 commented Jul 19, 2020

Also @Luap99, would you mind writing an issue to mention that TraverseChildren is not being considered for bash completion?

I added a commit here which should fix this and the bug we discovered (#1172).

@Luap99 Luap99 force-pushed the compSubCmd-TraverseChildren branch 3 times, most recently from c53431f to 4d3c1f2 Compare July 19, 2020 18:03
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions_test.go Outdated Show resolved Hide resolved
custom_completions.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

@Luap99 Nice! Just a couple of minor tweaks if you agree, and the suggestion for the extra test about using Traverse instead of Find.

But it works great both for bash and for zsh with go completions.

bash_completions.go Show resolved Hide resolved
Paul Holzinger added 2 commits July 20, 2020 20:13
The current custom completion logic does not complete
subcommands when a local flag is set. This is good unless
TraverseChildren is set to true where local flags
can be set on parent commands.

This commit allows subcommands to be completed
if TraverseChildren is set to true on the root cmd.

Closes spf13#1170

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
The current bash completion logic does not complete
subcommands when a local flag is set. There is also a bug
where subcommands are sometimes still getting completed. see: spf13#1172

If TraverseChildren is true we should allow subcommands
to be completed even if a local flag is set.

Closes spf13#1172

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@Luap99 Luap99 force-pushed the compSubCmd-TraverseChildren branch from 4d3c1f2 to a3adff6 Compare July 20, 2020 18:23
@Luap99
Copy link
Contributor Author

Luap99 commented Jul 20, 2020

@marckhouzam I have updated the code according to your suggestions.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

I believe this is correct and safe to merge.

/cc @jharshman @jpmcb

Thanks you @Luap99 !

@jpmcb
Copy link
Collaborator

jpmcb commented Aug 31, 2020

Thanks for this @Luap99!! Would you be able to provide some more context / a good way I can test this? I'll merge this once I've tried this out

@Luap99
Copy link
Contributor Author

Luap99 commented Aug 31, 2020

@jpmcb I run into this while working on containers/podman#6442. Podman has a local flag called remote on the root cmd to indicate that the command should be run against a remote endpoint. Since the root cmd has TraverseChildren set to true, this is a vaild cmd: podman --remote ps. I was wondering why the completion didn't offer subcmds if I tried something like this: podman --remote [TAB] and found this bug.

Unfortunately podman is not easy to set up. Therefore I can create a simple example program for you if you want to test it.

@Luap99
Copy link
Contributor Author

Luap99 commented Aug 31, 2020

package main

import (
	"fmt"
	"os"
	"path/filepath"
	"strings"

	"github.com/spf13/cobra"
)

var completionNoDesc = false
var remote = false

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

Bash:

$ source <(yourprogram completion bash)

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

Zsh:

$ source <(yourprogram completion zsh)

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

Fish:

$ yourprogram completion fish | source

# To load completions for each session, execute once:
$ yourprogram completion fish > ~/.config/fish/completions/yourprogram.fish
`,
	DisableFlagsInUseLine: true,
	ValidArgs:             []string{"bash", "zsh", "fish", "powershell", "bash-old"},
	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:              filepath.Base(os.Args[0]),
	TraverseChildren: true,
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("rootCmd called")
		fmt.Printf("remote %v\n", remote)
	},
}

var vargsCmd = &cobra.Command{
	Use:       "vargs",
	Short:     "Cmd with vargs and subcmd",
	ValidArgs: []string{"one", "two", "three"},
	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")
		fmt.Printf("remote %v\n", remote)
	},
}

// File filtering for the first argument
// Replaces MarkZshCompPositionalArgumentFile(1, string{"*.log", "*.txt"})
var filterFileCmd = &cobra.Command{
	Use:   "filterFile",
	Short: "filterFile [*.log,*.txt]",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) == 0 {
			return []string{"log", "txt"}, cobra.ShellCompDirectiveFilterFileExt
			//return nil, 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")
		fmt.Printf("remote %v\n", remote)
	},
}

// Turn off file completion for all arguments
var noFileCmd = &cobra.Command{
	Use:   "nofile",
	Short: "nofile",
	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")
		fmt.Printf("remote %v\n", remote)
	},
}

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

// Only allow directory name completion and only for the first argument
var dirOnlyCmd = &cobra.Command{
	Use:   "dirOnly",
	Short: "dirOnly [dirname]",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		if len(args) == 0 {
			return []string{"2"}, 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")
		fmt.Printf("remote %v\n", remote)
	},
}

// Only allow directory name from within a specified directory and only for the first argument
var subdirCmd = &cobra.Command{
	Use:   "subdir",
	Short: "subdir [sub-dirname]",
	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")
		fmt.Printf("remote %v\n", remote)
	},
}

// Test file completion when returning other completions
var compAndFileCmd = &cobra.Command{
	Use:   "compAndFile",
	Short: "compAndFile [sun|moon|<filename>]",
	ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
		var completions []string
		// If we don't filter on "toComplete", zsh and fish will not do file completion
		// even if the prefix typed by the user does not match the returned completions
		// e.g., ./testprog compAndFile f<TAB>
		// will only get file completion for bash, not zsh and fish, if we blindly return
		// the non-matching completions ("sun", "moon")
		for _, comp := range []string{"sun", "moon"} {
			if strings.HasPrefix(comp, toComplete) {
				completions = append(completions, comp)
			}
		}
		return completions, cobra.ShellCompDirectiveDefault
	},
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("compAndFile called")
		fmt.Printf("remote %v\n", remote)
	},
}

func setFlags() {
	rootCmd.PersistentFlags().String("persistent", "", "persistent flag")

	rootCmd.Flags().BoolVarP(&remote, "remote", "r", false, "bool flag")

}

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

	rootCmd.AddCommand(filterFileCmd)
	rootCmd.AddCommand(noFileCmd)
	rootCmd.AddCommand(fullFileCmd)
	rootCmd.AddCommand(dirOnlyCmd)
	rootCmd.AddCommand(subdirCmd)
	rootCmd.AddCommand(compAndFileCmd)

	rootCmd.AddCommand(vargsCmd)
	vargsCmd.AddCommand(childVargsCmd)

	setFlags()
	rootCmd.Execute()
}

@jpmcb Try this with TraverseChildren true and false on the root cmd and do something like this ./testprog --remote [TAB]

@Luap99
Copy link
Contributor Author

Luap99 commented Sep 7, 2020

@jpmcb Did you had the time to test this? Is anything else needed?

@jpmcb
Copy link
Collaborator

jpmcb commented Sep 9, 2020

Yes, I've tested this out with your example program, thank you very much for the context!! Everything looks good to me. Thanks for the work on this @Luap99 and @marckhouzam

@jpmcb jpmcb merged commit 50258f1 into spf13:master Sep 9, 2020
@Luap99
Copy link
Contributor Author

Luap99 commented Sep 9, 2020

@jpmcb Would you please so kind and take a look at #1162. It is a very small fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants