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.

Sign up for GitHub

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

Show the actual available flags in useline #2105

Closed
wants to merge 1 commit into from
Closed

Show the actual available flags in useline #2105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 97 additions & 4 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ type Command struct {
// line of a command when printing help or generating docs
DisableFlagsInUseLine bool

// DisableVerboseFlagsInUseLine will add only [flags] to the usage line of
// a command when printing help or generating docs instead of a more verbose
// form showing the actual available flags
DisableVerboseFlagsInUseLine bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is consistent with other options, but this change may visually break existing tools using this library. Should we use EnableVerboseFlagsInUsageLine so one can opt in to use the new feature?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an important point. Let's see where the final change lands and we can decide if we need to be less aggressive in pushing this change


// DisableSuggestions disables the suggestions based on Levenshtein distance
// that go along with 'unknown command' messages.
DisableSuggestions bool
Expand Down Expand Up @@ -1449,13 +1454,101 @@ func (c *Command) UseLine() string {
} else {
useline = use
}
return useline + c.uselineFlags(useline)
}

func (c *Command) uselineFlags(useline string) string {
if c.DisableFlagsInUseLine {
return useline
return ""
}
if c.HasAvailableFlags() && !strings.Contains(useline, "[flags]") {
useline += " [flags]"
if !c.HasAvailableFlags() {
return ""
}
return useline
if strings.Contains(useline, "[flags]") {
return ""
}
if c.DisableVerboseFlagsInUseLine {
return " [flags]"
}

included := map[*flag.Flag]struct{}{}
flagsLine := ""

c.flags.VisitAll(func(f *flag.Flag) {
if _, ok := included[f]; ok || f.Hidden {
return
}
included[f] = struct{}{}

rag := flagsFromAnnotation(c, f, requiredAsGroup)
me := flagsFromAnnotation(c, f, mutuallyExclusive)
or := flagsFromAnnotation(c, f, oneRequired)

if len(rag) > 0 {
gr := []string{}
for _, fl := range rag {
included[fl] = struct{}{}
gr = append(gr, shortUsage(fl))
}
flagsLine += " [" + strings.Join(gr, " ") + "]"
} else if len(me) > 0 {
gr := []string{}
for _, fl := range me {
included[fl] = struct{}{}
gr = append(gr, shortUsage(fl))
}
if sameFlags(me, or) {
flagsLine += " {" + strings.Join(gr, " | ") + "}"
} else {
flagsLine += " [" + strings.Join(gr, " | ") + "]"
}
} else if req, found := f.Annotations[BashCompOneRequiredFlag]; found && req[0] == "true" {
flagsLine += " " + shortUsage(f)
} else {
flagsLine += " [" + shortUsage(f) + "]"
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The code to list the short usage strings is repeated twice, making this function even more complicated and hard to follow. We have here many very short variable names (arg, me, or, gr, fl) and complex string formatting. It is pain to follow this code.

We can make it little better by extracting few helpers. I'm not sure about the names, they can be better.

func groupShortUsage(flags map[string]*flag.Flag, included map[*flag.Flag]struct{}) []string {
    gr := []string{}
    for _, fl := range flags {
        included[fl] = struct{}{}
        gr = append(gr, shortUsage(fl))
    }
    return gr
}

func isOneRequired(f *flag.Flag) bool {
    req, found := f.Annotations[BashCompOneRequiredFlag]
    return found && req[0] == "true"    
}

Now we have only 3 very short variables names which is easy to grasp, the rest is the obvious formatting.

		rag := flagsFromAnnotation(c, f, requiredAsGroup)
		me := flagsFromAnnotation(c, f, mutuallyExclusive)
		or := flagsFromAnnotation(c, f, oneRequired)

		if len(rag) > 0 {
			usages := groupShortUsage(rag, included)
			flagsLine += " [" + strings.Join(usages, " ") + "]"
		} else if len(me) > 0 {
			usages := groupShortUsage(me, included)
			if sameFlags(me, or) {
				flagsLine += " {" + strings.Join(usages, " | ") + "}"
			} else {
				flagsLine += " [" + strings.Join(usages, " | ") + "]"
			}
		} else if isOneRequired(f) {
			flagsLine += " " + shortUsage(f)
		} else {
			flagsLine += " [" + shortUsage(f) + "]"
		}

return flagsLine
}

func shortUsage(f *flag.Flag) (usage string) {
if f.Shorthand != "" {
usage = "-" + f.Shorthand
} else {
usage = "--" + f.Name
}

varname, _ := flag.UnquoteUsage(f)
if varname != "" {
usage += " " + varname
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Using bare return is not consistent with other code added.

}

func flagsFromAnnotation(c *Command, f *flag.Flag, name string) map[string]*flag.Flag {
flags := map[string]*flag.Flag{}
a := f.Annotations[name]
for _, s := range a {
for _, name := range strings.Split(s, " ") {
fl := c.flags.Lookup(name)
if fl != nil {
flags[name] = fl
}
}
}
return flags
}

func sameFlags(a, b map[string]*flag.Flag) bool {
if len(a) != len(b) {
return false
}
for k, v := range a {
if b[k] != v {
return false
}
}
return true
}

// DebugFlags used to determine which flags have been assigned to which commands
Expand Down
80 changes: 73 additions & 7 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ func TestPlugin(t *testing.T) {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, cmdHelp, "kubectl plugin [flags]")
checkStringContains(t, cmdHelp, "kubectl plugin [-h]")
checkStringContains(t, cmdHelp, "help for kubectl plugin")
}

Expand All @@ -398,7 +398,7 @@ func TestPluginWithSubCommands(t *testing.T) {
},
}

subCmd := &Command{Use: "sub [flags]", Args: NoArgs, Run: emptyRun}
subCmd := &Command{Use: "sub [-h]", Args: NoArgs, Run: emptyRun}
rootCmd.AddCommand(subCmd)

rootHelp, err := executeCommand(rootCmd, "-h")
Expand All @@ -415,7 +415,7 @@ func TestPluginWithSubCommands(t *testing.T) {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, childHelp, "kubectl plugin sub [flags]")
checkStringContains(t, childHelp, "kubectl plugin sub [-h]")
checkStringContains(t, childHelp, "help for sub")

helpHelp, err := executeCommand(rootCmd, "help", "-h")
Expand Down Expand Up @@ -975,7 +975,7 @@ func TestHelpCommandExecutedOnChildWithFlagThatShadowsParentFlag(t *testing.T) {
}

expected := `Usage:
parent child [flags]
parent child [--bar] [--baz] [--foo] [-h]

Flags:
--baz child baz usage
Expand Down Expand Up @@ -1053,11 +1053,11 @@ func TestHelpFlagInHelp(t *testing.T) {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "[flags]")
checkStringContains(t, output, "[-h]")
}

func TestFlagsInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}, DisableVerboseFlagsInUseLine: true}
output, err := executeCommand(rootCmd, "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
Expand All @@ -1066,6 +1066,72 @@ func TestFlagsInUsage(t *testing.T) {
checkStringContains(t, output, "[flags]")
}

func TestMutuallyExclusiveFlagsInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of func(*Command, []string) {}} we can use emptyRun

rootCmd.Flags().SortFlags = false
rootCmd.Flags().Bool("foo", false, "foo desc")
rootCmd.Flags().Bool("bar", false, "bar desc")
rootCmd.MarkFlagsMutuallyExclusive("foo", "bar")
output, err := executeCommand(rootCmd, "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "[--foo | --bar]")
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider more condensed format?

command [--foo|--bar]

popular tools like python argparse use the same format suggested, for example:

$ cat args.py 
import argparse
p = argparse.ArgumentParser()
me = p.add_mutually_exclusive_group()
me.add_argument("--foo")
me.add_argument("--bar")
p.parse_args()

$ python args.py -h | head -1
usage: args.py [-h] [--foo FOO | --bar BAR]

}

func TestMutuallyExclusiveRequiredFlagsInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
rootCmd.Flags().SortFlags = false
rootCmd.Flags().Bool("foo", false, "foo desc")
rootCmd.Flags().Bool("bar", false, "bar desc")
rootCmd.MarkFlagsMutuallyExclusive("foo", "bar")
rootCmd.MarkFlagsOneRequired("foo", "bar")
output, err := executeCommand(rootCmd, "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "{--foo | --bar}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to chose {} for required choice from mutually exclusive arguments?

python argparse uses () for the same:

$ cat args.py 
import argparse
p = argparse.ArgumentParser()
me = p.add_mutually_exclusive_group(required=True)
me.add_argument("--foo")
me.add_argument("--bar")
p.parse_args()

$ python args.py -h | head -1
usage: args.py [-h] (--foo FOO | --bar BAR)

}

func TestRequiredFlagInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
rootCmd.Flags().Bool("foo", false, "foo desc")
rootCmd.MarkFlagRequired("foo")
output, err := executeCommand(rootCmd, "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, " --foo ")
}

func TestRequiredTogetherFlagsInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
rootCmd.Flags().SortFlags = false
rootCmd.Flags().Bool("foo", false, "foo desc")
rootCmd.Flags().Bool("bar", false, "bar desc")
rootCmd.MarkFlagsRequiredTogether("foo", "bar")
output, err := executeCommand(rootCmd, "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "[--foo --bar]")
}

func TestFlagWithArgInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
rootCmd.Flags().String("output", "", "the output `file`")
output, err := executeCommand(rootCmd, "--help")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "[--output file]")
}

func TestHelpExecutedOnNonRunnableChild(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
childCmd := &Command{Use: "child", Long: "Long description"}
Expand Down Expand Up @@ -2141,7 +2207,7 @@ func TestFlagErrorFuncHelp(t *testing.T) {
}

expected := `Usage:
c [flags]
c [--help]

Flags:
--help help for c
Expand Down