-
Notifications
You must be signed in to change notification settings - Fork 60
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
Comments
|
Sorry, |
Right. So, the approach is basically a visitor pattern — a function which accepts a // 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? |
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. |
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. |
@josharian Where'd you land on this one? What could I have given you to make it better? |
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. |
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? |
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.) |
Understood. I think it would be surprising if this behavior was the default, but it may make sense as something to opt-in to. |
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
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
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 |
One way might be to have a Command accept more than one FlagSet. |
I hope this will be addressed by #113. |
I often find myself repeating code like this:
That way:
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.)
The text was updated successfully, but these errors were encountered: