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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 89 additions & 2 deletions command.go
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:
{{$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"
func (c *Command) UsageByFlagHelpGroupID(groupID string) string {
if groupID == "global" {
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

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!

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
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}

rootCmd.AddCommand(child)

b := "b"
s := "s"
i := "i"
g := "g"

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)
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