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

Conversation

pedromotita
Copy link
Contributor

@pedromotita pedromotita commented Mar 7, 2024

Description

Flags can now be split into groups in usage text:

$ root child --help

Usage:
  root child [flags]

Flags:
  -h, --help    help for child
      --i int   int flag

Grouped Flags:
      --b          bool flag
      --s string   string flag

Global Flags:
      --g string   global flag

Related Issues

Implementation Details

The API for this feature was based on this comment. Check it out :)

@pedromotita pedromotita mentioned this pull request Mar 7, 2024
@CLAassistant
Copy link

CLAassistant commented Mar 7, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@paulopacitti paulopacitti left a comment

Choose a reason for hiding this comment

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

That's a nice addition to cobra!

if groupID == "" {
fs.AddFlag(f)
}
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

Copy link

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

Thank you for this PR, this is really appreciated!

I tested it and it works fine:

$ cat go.mod 
module github.com/eiffel-fl/group-flags-example

go 1.21.7

require github.com/spf13/cobra v1.8.0

require (
        github.com/inconshreveable/mousetrap v1.1.0 // indirect
        github.com/spf13/pflag v1.0.5 // indirect
)

replace github.com/spf13/cobra => github.com/pedromotita/cobra v0.0.0-20240307182157-e9498ae28a51
$ cat main.go 
package main

import (
        "github.com/spf13/cobra"
)

func main() {
        child := &cobra.Command{Use: "child", Run: func (*cobra.Command, []string) {}}
        rootCmd := &cobra.Command{Use: "root", Run: func (*cobra.Command, []string) {}}

        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 := cobra.Group{ID: "groupId", Title: "GroupTitle"}

        child.AddFlagHelpGroup(&group)

        _ = child.AddFlagToHelpGroupID(b, group.ID)
        _ = child.AddFlagToHelpGroupID(s, group.ID)
        rootCmd.Execute()
}%
$  go run . child --help
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

I have some small comments.

Best regards.

Comment on lines +1412 to +1414
if groupID == "" {
fs.AddFlag(f)
}
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 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.


// 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().

}

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

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.

@jpmcb jpmcb self-requested a review April 19, 2024 13:34
@jpmcb jpmcb added area/cobra-command Core `cobra.Command` implementations size/XL Denotes a PR that exceeds 200 lines. Caution! go Pull requests that update Go code labels Apr 19, 2024
@jzelinskie
Copy link

jzelinskie commented May 10, 2024

I noticed an edge case for a library I wrote to implement this in the past, which is that you probably want to hide a group's usage if all of the flags are hidden.

I'm not sure if you've accounted for this scenario.

{{.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.

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.


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.

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?

Comment on lines +1412 to +1414
if groupID == "" {
fs.AddFlag(f)
}
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 = ""

// 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" {
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.

}

// 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()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cobra-command Core `cobra.Command` implementations go Pull requests that update Go code size/XL Denotes a PR that exceeds 200 lines. Caution!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

group flags for a subcommand
8 participants