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

feat: add flag help groups #2117

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
91 changes: 89 additions & 2 deletions command.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (

const (
FlagSetByCobraAnnotation = "cobra_annotation_flag_set_by_cobra"
FlagHelpGroupAnnotation = "cobra_annotation_flag_help_group"
CommandDisplayNameAnnotation = "cobra_annotation_command_display_name"
)

Expand Down Expand Up @@ -145,6 +146,9 @@ type Command struct {
// groups for subcommands
commandgroups []*Group

// groups for flags in usage text.
flagHelpGroups []*Group

// args is actual args parsed from flags.
args []string
// flagErrorBuf contains all error messages from pflag.
Expand Down Expand Up @@ -568,13 +572,22 @@ Available Commands:{{range $cmds}}{{if (or .IsAvailableCommand (eq .Name "help")
{{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{if not .AllChildCommandsHaveGroup}}
Additional Commands:{{range $cmds}}{{if (and (eq .GroupID "") (or .IsAvailableCommand (eq .Name "help")))}}
{{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{end}}{{end}}{{if .HasAvailableLocalFlags}}
{{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{end}}{{end}}{{$cmd := .}}{{if eq (len .FlagHelpGroups) 0}}{{if .HasAvailableLocalFlags}}
Flags:
{{.LocalFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasAvailableInheritedFlags}}
Global Flags:
{{.InheritedFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasHelpSubCommands}}
{{.InheritedFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{else}}{{$flags := .LocalFlags}}{{range $helpGroup := .FlagHelpGroups}}{{if not (eq (len ($cmd.UsageByFlagHelpGroupID "")) 0)}}
Flags:
Copy link
Contributor

Choose a reason for hiding this comment

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

"Flags" and "Title Flags" are not consistent. It would be nice if all flags are grouped, and so every group has a meaning full name.

If only some flags are grouped, maybe the rest (the un-grouped flags) can appear last instead of first?

We can have 2 reasons to group flags:

  • Move down related flags that are not common to the end
  • Move up important flags that are very common to make them easier to find

Example:

Common flags:
  --foo int ...
  --bar string ...

Security flags:
  --insecure    disable TLS certificate verification
  --ca-file     path to ca certificate

Global flags:
   ...

We probably could use example from real commands to evaluate this.

{{$cmd.UsageByFlagHelpGroupID "" | trimTrailingWhitespaces}}{{end}}
{{.Title}} Flags:

Choose a reason for hiding this comment

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

This can cause problems if localization support is added.
{{.Title}}: is probably better.

{{$cmd.UsageByFlagHelpGroupID $helpGroup.ID | trimTrailingWhitespaces}}{{if not (eq (len ($cmd.UsageByFlagHelpGroupID "global")) 0)}}
Global Flags:
{{$cmd.UsageByFlagHelpGroupID "global" | trimTrailingWhitespaces}}{{end}}{{end}}{{end}}{{if .HasHelpSubCommands}}
Additional help topics:{{range .Commands}}{{if .IsAdditionalHelpTopicCommand}}
{{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{if .HasAvailableSubCommands}}
Expand Down Expand Up @@ -1336,6 +1349,80 @@ func (c *Command) Groups() []*Group {
return c.commandgroups
}

// FlagHelpGroups returns a slice of the command's flag help groups
func (c *Command) FlagHelpGroups() []*Group {
return c.flagHelpGroups
}

// AddFlagHelpGroup adds one more flag help group do the command. Returns an error if the Group.ID is empty,
// or if the "global" reserved ID is used
func (c *Command) AddFlagHelpGroup(groups ...*Group) error {

Choose a reason for hiding this comment

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

I know that Group is already used in cobra, but I am wondering if we should not use Group instead of HelpGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a previous discussion on why to use cobra's Group in this thread. Take a look 😉

Choose a reason for hiding this comment

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

I was only referring to naming here, i.e. AddFlagGroup() instead of AddFlagHelperGroup().

for _, group := range groups {
if len(group.ID) == 0 {
return fmt.Errorf("flag help group ID must have at least one character")
}

if group.ID == "global" {
return fmt.Errorf(`"global" is a reserved flag help group ID`)
}

c.flagHelpGroups = append(c.flagHelpGroups, group)
}

return nil
}

func (c *Command) hasFlagHelpGroup(groupID string) bool {

Choose a reason for hiding this comment

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

Instead of ID, should you use category?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand. Do you mean renaming groupID to category?

I prefer groupID to make it clear we are dealing with Group here

Choose a reason for hiding this comment

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

I don't understand. Do you mean renaming groupID to category?

Yes.

I prefer groupID to make it clear we are dealing with Group here

Let's stick with groupID.

for _, g := range c.flagHelpGroups {
if g.ID == groupID {
return true
}
}

return false
}

// AddFlagToHelpGroupID adds associates a flag to a groupID. Returns an error if the flag or group is non-existent
func (c *Command) AddFlagToHelpGroupID(flag, groupID string) error {

Choose a reason for hiding this comment

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

It could be useful to have some option to add multiple flags in one call (like in AddFlagHelpGroup)

Choose a reason for hiding this comment

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

I second this point, having a variadic function would be welcomed!
The implementation could be something like this:

func (c *Command) AddFlagsToHelpGroupID(groupID string, flags ...string) error {
	for _, flag := range flags  {
		err := AddFlagToHelpGroupID(flag, groupID)
		if err != nil {
			return err
		}
	}
	
	return nil
}

lf := c.Flags()

if !c.hasFlagHelpGroup(groupID) {
return fmt.Errorf("no such flag help group: %v", groupID)
}

err := lf.SetAnnotation(flag, FlagHelpGroupAnnotation, []string{groupID})
if err != nil {
return err
}

return nil
}

// UsageByFlagHelpGroupID returns the command flag's usage split by flag help groups. Flags without groups associated
// will appear under "Flags", and inherited flags will appear under "Global Flags"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that the code return help for single flag group, not help split by groups. Function comment needs update?

Also maybe rename the function to UsageForHelpGroup()?

func (c *Command) UsageByFlagHelpGroupID(groupID string) string {
if groupID == "global" {
Copy link
Contributor

Choose a reason for hiding this comment

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

A constant for "global" would be nice.

return c.InheritedFlags().FlagUsages()
}

fs := &flag.FlagSet{}

c.LocalFlags().VisitAll(func(f *flag.Flag) {
if _, ok := f.Annotations[FlagHelpGroupAnnotation]; !ok {
if groupID == "" {
fs.AddFlag(f)
}
Comment on lines +1412 to +1414
Copy link

@eiffel-fl eiffel-fl Mar 12, 2024

Choose a reason for hiding this comment

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

I am not sure to understand why you are adding flags without group to this flag set, can you please shed some light?

Copy link
Contributor Author

@pedromotita pedromotita Mar 15, 2024

Choose a reason for hiding this comment

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

This is the solution I've found for printing the usage of flags that have not been added to any flagHelpGroup.

If you see the template on Command.UsageTemplate, I use UsageByFlagHelpGroupID to print flag usages when len(c.FlagHelpGroups) != 0. But I still have to cover the case where there are flags not attached to any group, which is the case of the i flag in your example. I guarantee that there wont be any empty groupID by returning an error at AddFlagHelpGroup

The same goes for InheritedFlags and the global reserved group ID.

Choose a reason for hiding this comment

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

So, this code basically avoids having orphan flags without group.
This makes sense, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a constant for the special empty group id?

const NoGroup = ""

return

Choose a reason for hiding this comment

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

Isn't return "" better than returning nil in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This return statement is from VisitAll() closure. It's not actually returning any values, but working as a control flow statement, so we don't access an out of bound index at f.Annotations[FlagHelpGroupsAnnotation] bellow

Choose a reason for hiding this comment

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

you're right, my mistake! Resolving it

}

if id := f.Annotations[FlagHelpGroupAnnotation][0]; id == groupID {
fs.AddFlag(f)
}
})

return fs.FlagUsages()
}

// AllChildCommandsHaveGroup returns if all subcommands are assigned to a group
func (c *Command) AllChildCommandsHaveGroup() bool {
for _, sub := range c.commands {
Expand Down
76 changes: 76 additions & 0 deletions command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,82 @@ func TestPersistentRequiredFlagsWithDisableFlagParsing(t *testing.T) {
}
}

func TestFlagHelpGroups(t *testing.T) {

t.Run("add flag to non-existing flag help group", func(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}
b := "b"

rootCmd.Flags().Bool(b, false, "bool flag")

err := rootCmd.AddFlagToHelpGroupID(b, "id")
if err == nil {
t.Error("Expected error when adding a flag to non-existent flag help group")
}
})

t.Run("add non-existing flag to flag help group", func(t *testing.T) {
rootCmd := &Command{Use: "root", Run: emptyRun}

group := Group{ID: "id", Title: "GroupTitle"}
rootCmd.AddFlagHelpGroup(&group)

err := rootCmd.AddFlagToHelpGroupID("", "id")
if err == nil {
t.Error("Expected error when adding a non-existent flag to flag help group")
}

})

t.Run("add flag to flag help group", func(t *testing.T) {
child := &Command{Use: "child", Run: emptyRun}
rootCmd := &Command{Use: "root", Run: emptyRun}
Copy link
Contributor

Choose a reason for hiding this comment

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

child and rootCmd are not consistent. I think changing rootCmd to root will help.


rootCmd.AddCommand(child)

b := "b"
s := "s"
i := "i"
g := "g"
Copy link
Contributor

Choose a reason for hiding this comment

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

These variable do not help much. Maybe using more meaningful names could be better.


child.Flags().Bool(b, false, "bool flag")
child.Flags().String(s, "", "string flag")
child.Flags().Int(i, 0, "int flag")
rootCmd.PersistentFlags().String(g, "", "global flag")

group := Group{ID: "groupId", Title: "GroupTitle"}

child.AddFlagHelpGroup(&group)

_ = child.AddFlagToHelpGroupID(b, group.ID)
_ = child.AddFlagToHelpGroupID(s, group.ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

This add the flag to the help group - not the to help group id. Rename to AddFlagToHelpGroup?

x := `Usage:
root child [flags]
Flags:
-h, --help help for child
--i int int flag
GroupTitle Flags:
--b bool flag
--s string string flag
Global Flags:
--g string global flag
`

got, err := executeCommand(rootCmd, "help", "child")
if err != nil {
t.Errorf("Unexpected error: %v", err)
}

if got != x {
t.Errorf("Help text mismatch.\nExpected:\n%s\n\nGot:\n%s\n", x, got)
}
})

}

func TestInitHelpFlagMergesFlags(t *testing.T) {
usage := "custom flag"
rootCmd := &Command{Use: "root"}
Expand Down