Add group flags #1778

knqyf263 · 2022-08-18T09:20:12Z

Description

Add group flags

Example

main.go

package main

import (
	"fmt"

	"github.com/spf13/cobra"
)

func main() {
	var output string
	var rootCmd = &cobra.Command{
		Use:   "root [sub]",
		Short: "My root command",
		Run: func(cmd *cobra.Command, args []string) {
			fmt.Printf("Output: %s\n", output)
		},
	}
	rootCmd.Flags().Int("replicas", 0, "replicas")
	rootCmd.NamedFlags("cache").String("dir", "", "cache dir")
	rootCmd.NamedFlags("cache").Duration("max-age", 0, "cache ttl")
	rootCmd.NamedFlags("result").StringVarP(&output, "output", "o", "", "output format")
	rootCmd.NamedFlags("result").StringP("filename", "f", "", "filename")

	rootCmd.SetArgs([]string{"--output", "dummy.txt"})
	rootCmd.Execute()
	fmt.Println()
	rootCmd.SetArgs([]string{"--help"})
	rootCmd.Execute()
}

$ go run main.go
Output: dummy.txt

My root command

Usage:
  root [sub] [flags]

Flags:
  -h, --help           help for root
      --replicas int   replicas

Cache Flags:
      --dir string         cache dir
      --max-age duration   cache ttl

Result Flags:
  -f, --filename string   filename
  -o, --output string     output format

CLAassistant · 2022-08-18T09:20:16Z

All committers have signed the CLA.

knqyf263 · 2022-08-18T09:20:49Z

named_flag_sets.go

@@ -0,0 +1,89 @@
+package cobra


Should this file be in spf13/pflag?

knqyf263 · 2022-08-18T09:21:41Z

command.go

+}
+
+// NamedFlags returns the specific named FlagSet that applies to this command.
+func (c *Command) NamedFlags(name string) *flag.FlagSet {


NamedFlags doesn't support persistent flags at the moment.

Do you have any idea of what should be done to add this support?

knqyf263 · 2022-08-18T09:23:36Z

@marckhouzam I don't think this is the best implementation, but I hope this is a good starting point for discussion.

I borrowed an idea of NamedFlagSets from Kubernetes.
https://github.com/kubernetes/component-base/blob/b5a495af30a7bb04642ce82f4816b47e75f78dbe/cli/flag/sectioned.go#L33-L41

github-actions · 2022-10-18T00:14:39Z

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
You can:
Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening.

github-actions · 2023-01-19T00:08:45Z

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed.
You can:
Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interested in reopening.

EraYaN · 2023-06-26T11:21:31Z

This would still be a very valuable feature and should not go stale.

eiffel-fl

Hi.

I tested this PR and it works fine:

$ tail go.mod
go 1.18

require github.com/spf13/cobra v1.7.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/knqyf263/cobra v1.5.1-0.20220818091539-ec6fe6b61cd4
$ go run .
Output: dummy.txt

My root command

Usage:
  root [sub] [flags]

Flags:
  -h, --help           help for root
      --replicas int   replicas

Cache Flags:
      --dir string         cache dir
      --max-age duration   cache ttl

Result Flags:
  -f, --filename string   filename
  -o, --output string     output format

I also applied the given patch to latest spf13/cobra main branch and it applies cleanly.
This contribution would really help the community once merged, so I think it is totally worth it continuing to work on it.
I have some comments regarding the code but nothing critical with regard to my basic knowledge of spf13/cobra.

Best regards.

eiffel-fl · 2023-10-18T11:10:40Z

command.go

+}
+
+// NamedFlags returns the specific named FlagSet that applies to this command.
+func (c *Command) NamedFlags(name string) *flag.FlagSet {


Do you have any idea of what should be done to add this support?

eiffel-fl · 2023-10-18T11:13:21Z

command.go

+// NamedFlagSets returns all the named FlagSet that applies to this command.
+func (c *Command) NamedFlagSets() *NamedFlagSets {
+	if c.lnamedFlagSets == nil {
+		c.lnamedFlagSets = NewNamedFlagSets(c.Name(), flag.ContinueOnError)


Should the flat.ContinueOnError be hardcoded?

eiffel-fl · 2023-10-18T11:15:48Z

named_flag_sets.go

+	if nfs.flagSets == nil {
+		nfs.flagSets = map[string]*pflag.FlagSet{}
+	}


Should this operation be done in NewNamedFlagSets()?

eiffel-fl · 2023-10-18T11:19:43Z

named_flag_sets.go

+}
+
+func (nfs *NamedFlagSets) FlagUsagesWrapped(cols int) string {
+	var buf bytes.Buffer


Would it be better to use a string builder?

eiffel-fl · 2023-10-18T11:22:05Z

named_flag_sets.go

+		if cols > 24 {
+			i := strings.Index(s, zzz)
+			lines := strings.Split(s[:i], "\n")
+			fmt.Fprint(&buf, strings.Join(lines[:len(lines)-1], "\n"))
+			fmt.Fprintln(&buf)


Is this code really needed? And why 24?

pedromotita · 2023-11-25T22:41:20Z

Hi, is there any updates do this PR status? It's a really nice feature. A lot of projects would benefit from it

knqyf263 · 2023-11-26T03:56:10Z

@pedromotita I wanted to kick off the discussion, but I've not received any response from maintainers.

eiffel-fl · 2023-11-27T09:00:17Z

Hi.

@pedromotita I wanted to kick off the discussion, but I've not received any response from maintainers.

I am not in maintainers heads, but you can try to publish this PR as ready for review and they may take a look (cost nothing to try by the way).

Best regards.

marckhouzam · 2023-12-18T01:17:06Z

Thanks @knqyf263 for your patience (and everyone else interested in this), I will start looking at this PR.

marckhouzam · 2023-12-18T02:09:22Z

There is a PR to support this in spf13/pflag directly: spf13/pflag#339
I'm mentioning this as it can be a reference, however, there has been no movement in that project for years, so I think we should continue adding support in Cobra instead.

marckhouzam · 2023-12-18T02:32:55Z

@knqyf263 Although I think your API is very elegant, I'm concerned it complicates the code in Cobra and would make maintenance more difficult. I'm worried about creating yet another "type" of flags and then have: local flags, inherited flags and named flags.

The grouping of flags in the help does not affect their actual behaviour. So from a behavioural perspective we still have either local or inherited flags. I would prefer keeping those two groups and adding an independent concept of "flag group" on top of this.

For better or worse we already have an API for command groups (see doc here), so it may be good for users of Cobra to have a similar API for flag groups.

With that in mind, what would you think of an API that looked something like:

var rootCmd = &cobra.Command{
	Use:   "root [sub]",
	Short: "My root command",
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Printf("Root command")
	},
}

// Create flags exactly like is currently done
rootCmd.Flags().Int("replicas", 0, "replicas")
rootCmd.Flags.()String("dir", "", "cache dir")
rootCmd.Flags().Duration("max-age", 0, "cache ttl")
rootCmd.PersistentFlags().StringVarP(&output, "output", "o", "", "output format")
rootCmd.PersistentFlags().StringP("filename", "f", "", "filename")

// Add new, separate knowledge for grouping flags in the help output
err = rootCmd.AddFlagHelpGroup(cobra.Group{ID: "group1", "Title": "cache"}, cobra.Group{ID: "group2", "Title": "result"})
err = rootCmd.AddFlagToHelpGroupID("dir", "group1")
err = rootCmd.AddFlagToHelpGroupID("max-age", "group1")
err = rootCmd.AddFlagToHelpGroupID("output", "group2")
err = rootCmd.AddFlagToHelpGroupID("filename", "group2")

Notice the use of the word "Help" to qualify the flag groups; I think we need something along those lines because Cobra has a concept of flag group for mutual exclusion and such things.

I admit I like your API better, but I am hoping that something along the lines of the above will be safer to implement. And it should handle global flags transparently, I'm hoping.

Hopefully this can start a discussion leading to something we all agree is useful.

pedromotita · 2023-12-25T21:17:26Z

@knqyf263 any thoughts on @marckhouzam API proposal? He has a fair point

Let us know if you're still willing to work on this PR. I'm new to the community, but would be very happy to help 😄

pedromotita · 2024-03-05T18:34:56Z

@marckhouzam I've been trying to wrap my head around a solution using your API suggestion. The main problem is that flag.FlagSet has no reference to cobra.Group.

Whereas dealing with Command we can use {{if (and (eq .GroupID $group.ID) ... }} in the template, we can't do that for flags... We could create a map do tie a Group.ID to a flag.FlagSet, but it's not a elegant solution at all...

The more I think about this feature, the more I get convinced part of it should be implemented in spf13/pflag package 😢

marckhouzam · 2024-03-06T00:43:59Z

I believe flags have an annotation field. Maybe you can use that to associate the group Id?

pedromotita · 2024-03-07T18:34:52Z

@marckhouzam I had not thought of that. Thanks!

I opened the PR #2117 with your API proposal and some test cases :)

Add group flags

ec6fe6b

github-actions bot added the size/L Denotes a PR that chanes 100-199 lines label Aug 18, 2022

knqyf263 commented Aug 18, 2022

View reviewed changes

named_flag_sets.go

@@ -0,0 +1,89 @@

package cobra

Copy link

Author

knqyf263 Aug 18, 2022

Choose a reason for hiding this comment

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

Should this file be in spf13/pflag?

knqyf263 commented Aug 18, 2022

View reviewed changes

knqyf263 mentioned this pull request Aug 18, 2022

group flags for a subcommand #1327

Open

blanquicet mentioned this pull request Sep 9, 2022

cli: Add flag groups to make help output more readable inspektor-gadget/inspektor-gadget#935

Open

github-actions bot added the lifecycle/stale label Oct 18, 2022

github-actions bot added the lifecycle/rotten label Nov 18, 2022

github-actions bot closed this Nov 18, 2022

marckhouzam reopened this Nov 18, 2022

github-actions bot removed lifecycle/rotten labels Nov 19, 2022

abitrolly mentioned this pull request Nov 23, 2022

Grouping podman build options containers/buildah#4431

Closed

github-actions bot added the lifecycle/stale label Jan 19, 2023

FedeDP mentioned this pull request Feb 6, 2023

new: dynamic builders falcosecurity/driverkit#244

Merged

stuartpa mentioned this pull request May 26, 2023

usage formatting microsoft/go-sqlcmd#360

Merged

brmzkw mentioned this pull request Jul 28, 2023

Group flags by type koyeb/koyeb-cli#99

Open

eiffel-fl reviewed Oct 18, 2023

View reviewed changes

AmadeusK525 mentioned this pull request Nov 6, 2023

[CLI] Move Persistent Flags to root MagaluCloud/magalu#45

Closed

mauriciovasquezbernal mentioned this pull request Nov 29, 2023

kubectl-gadget: Add --deploy flag to all commands inspektor-gadget/inspektor-gadget#2196

Closed

marckhouzam removed the lifecycle/stale label Dec 23, 2023

pedromotita mentioned this pull request Mar 8, 2024

feat: add flag help groups #2117

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add group flags #1778

Add group flags #1778

knqyf263 commented Aug 18, 2022

CLAassistant commented Aug 18, 2022 •

edited

Loading

knqyf263 Aug 18, 2022

knqyf263 Aug 18, 2022

eiffel-fl Oct 18, 2023

knqyf263 commented Aug 18, 2022

github-actions bot commented Oct 18, 2022

github-actions bot commented Jan 19, 2023

EraYaN commented Jun 26, 2023

eiffel-fl left a comment

eiffel-fl Oct 18, 2023

eiffel-fl Oct 18, 2023

eiffel-fl Oct 18, 2023

eiffel-fl Oct 18, 2023

eiffel-fl Oct 18, 2023

pedromotita commented Nov 25, 2023

knqyf263 commented Nov 26, 2023

eiffel-fl commented Nov 27, 2023

marckhouzam commented Dec 18, 2023

marckhouzam commented Dec 18, 2023

marckhouzam commented Dec 18, 2023

pedromotita commented Dec 25, 2023

pedromotita commented Mar 5, 2024

marckhouzam commented Mar 6, 2024

pedromotita commented Mar 7, 2024

Add group flags #1778

Are you sure you want to change the base?

Add group flags #1778

Conversation

knqyf263 commented Aug 18, 2022

Description

Example

CLAassistant commented Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 commented Aug 18, 2022

github-actions bot commented Oct 18, 2022

github-actions bot commented Jan 19, 2023

EraYaN commented Jun 26, 2023

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedromotita commented Nov 25, 2023

knqyf263 commented Nov 26, 2023

eiffel-fl commented Nov 27, 2023

marckhouzam commented Dec 18, 2023

marckhouzam commented Dec 18, 2023

marckhouzam commented Dec 18, 2023

pedromotita commented Dec 25, 2023

pedromotita commented Mar 5, 2024

marckhouzam commented Mar 6, 2024

pedromotita commented Mar 7, 2024

CLAassistant commented Aug 18, 2022 •

edited

Loading