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

Feature request: multi-level flags #76

Open
josharian opened this issue Feb 24, 2021 · 13 comments
Open

Feature request: multi-level flags #76

josharian opened this issue Feb 24, 2021 · 13 comments

Comments

@josharian
Copy link

I often find myself repeating code like this:

var verbose bool

func init() {
	all := []*flag.FlagSet{ /* list of FlagSets for different sub-commands and sub-sub-commands */ }
	for _, fs := range all {
		fs.BoolVar(&verbose, "v", false, "print debug information")
	}
}

That way:

$ cmd -v sub sub1
$ cmd sub -v sub1
$ cmd sub sub1 -v

all do the same thing. (And it's not just -v. I find it frustrating as a user to have to figure out at which level a flag goes, so I generally want to be able to put them anywhere.)

I'd like it if ff made this easier to accomplish.

I'm not sure what the API would look like but it'd be nice if there was a way to say "these flags can be set by any subcommand of this command".

Or maybe always do that and complain if there are any collisions? (That will also help avoid confusing UX in clients for which some flag means different things depending on where on the command line you add it.)

@peterbourgon
Copy link
Owner

ff or ffcli?

@josharian
Copy link
Author

Sorry, ffcli.

@peterbourgon
Copy link
Owner

peterbourgon commented Feb 24, 2021

Right. So, the approach is basically a visitor pattern — a function which accepts a *flag.FlagSet and registers the global flags within it. It's demonstrated in the objectctl example. RegisterFlags is the visitor

// RegisterFlags registers the flag fields into the provided flag.FlagSet. This
// helper function allows subcommands to register the root flags into their
// flagsets, creating "global" flags that can be passed after any subcommand at
// the commandline.
func (c *Config) RegisterFlags(fs *flag.FlagSet) {
	fs.StringVar(&c.Token, "token", "", "secret token for object API")
	fs.BoolVar(&c.Verbose, "v", false, "log verbose output")
}

which is invoked on the e.g. create subcommand like this

	fs := flag.NewFlagSet("objectctl create", flag.ExitOnError)
	fs.BoolVar(&cfg.overwrite, "overwrite", false, "overwrite existing object, if it exists")
	rootConfig.RegisterFlags(fs)

	return &ffcli.Command{
		Name:       "create",
		ShortUsage: "objectctl create [flags] <key> <value data...>",
		ShortHelp:  "Create or overwrite an object",
		FlagSet:    fs,
		Exec:       cfg.Exec,
	}

and makes the global flag values accessible like this

// Exec function for this command.
func (c *Config) Exec(ctx context.Context, args []string) error {

	. . .

	if c.rootConfig.Verbose {
		fmt.Fprintf(c.out, "create %q OK\n", key)
	}

Does this serve the purpose for you?

@josharian
Copy link
Author

Let me try migrating to v3 and playing with it a bit. One of the things I appreciate about ffcli is that most of the setup is declarative (I use top level vars). It looks like this'll require more glue code, like my init statement approach above, which I was hoping to avoid. I was rather drawn to automatic propagation. But I'll see how that approach goes, thanks.

@peterbourgon
Copy link
Owner

This or something like it is useful in ~every ffcli CLI. I'd love to find a way to make it easier — without expanding the surface area of ffcli.Command, which is already too wide. I wonder if a helper function or type could do the job.

@peterbourgon
Copy link
Owner

@josharian Where'd you land on this one? What could I have given you to make it better?

@josharian
Copy link
Author

I got distracted. :( I still suspect I’d like automatic propagation best, but I haven’t spent any time digging or thinking about it yet.

@peterbourgon
Copy link
Owner

Concretely, automatic propagation would mean defining or marking specific flags in a Command's FlagSet such that they're automatically registered in the FlagSets of all Subcommands, transitively?

@josharian
Copy link
Author

Yes. Or not even requiring marking, and just doing it.

Note that one way to implement is to try all flag sets of all ancestors, as opposed to registering additional flags. I believe that the only user-visible difference is whether -h shows all flags, including ancestor flags, or just flags specific to that subcommand. I lean weakly towards just subcommand-specific flags. The other advantage to this implementation approach is that it makes it clear what happens when an ancestor and a subcommand both define the same flag: the subcommand takes precedence. (The other reasonable approach is failure.)

@peterbourgon
Copy link
Owner

Understood. I think it would be surprising if this behavior was the default, but it may make sense as something to opt-in to.

stefanhengl added a commit to sourcegraph/zoekt that referenced this issue Feb 18, 2022
This adds a subcommand "debug" to indexserver.

The new subcommand contains all debug commands that were previously
handled as flags. This means that this commit changes the API but only for
debug command. There is no change in how we start indexserver.

The debug commands now live in a separate file "debug.go" and don't polute the
defintion of the server anymore. I used the vistor pattern as described
here peterbourgon/ff#76 to reuse the root
flags for some of the debug commands.

Other changes:
- removed deprecated flag exp-git-index
- removed second argument of setupTmpDir because it was not used
- sorted imports in main.go

How to review:
The diff is actually more readable than it appears at first glance. I
recommend to start at the bottom of main.go. Most of the changes in
main.go come from deleting things relating to debug commands and
changing the parameters from pointers to values.

Test Plan:
- I ran a local instance of Sourcegraph with a verion of Zoekt based on
this PR. Start up and indexing were successful.
- I built a binary for zoekt-sourcegraph-indexserver, copied it to a
production instance and tried out all debug commands
stefanhengl added a commit to sourcegraph/zoekt that referenced this issue Feb 21, 2022
This adds a subcommand "debug" to indexserver.

The new subcommand contains all debug commands that were previously
handled as flags. This means that this commit changes the API but only for
debug command. There is no change in how we start indexserver.

The debug commands now live in a separate file "debug.go" and don't polute the
defintion of the server anymore. I used the vistor pattern as described
here peterbourgon/ff#76 to reuse the root
flags for some of the debug commands.

Other changes:
- removed deprecated flag exp-git-index
- removed second argument of setupTmpDir because it was not used
- sorted imports in main.go
@mfridman
Copy link

mfridman commented Apr 7, 2023

This is a solid idea and if one could explicitly mark a flag as "inheritable" then it would simplify a lot of the boilerplate and glue code.

An effective way to surface this to users may be to display 2 flag sections:

USAGE
  gh run <command> [flags]

AVAILABLE COMMANDS
  cancel:      Cancel a workflow run
  ...

FLAGS
  -R, --repo [HOST/]OWNER/REPO   Select another repository using the [HOST/]OWNER/REPO format

INHERITED FLAGS
  --help   Show help for command

Example from gh CLI using cobra.

@peterbourgon
Copy link
Owner

One way might be to have a Command accept more than one FlagSet.

@peterbourgon
Copy link
Owner

I hope this will be addressed by #113.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants