peterbourgon / ff

Flags-first package for configuration
Apache License 2.0
1.37k stars 59 forks source link

ffcli.DefaultUsageFunc: Support flag help placeholders #106

Closed abhinav closed 1 year ago

abhinav commented 1 year ago

(This is an arguably uncontroversial part of the proposal in #105. Its's a new feature, not a significant change in behavior.)

Currently, ffcli.DefaultUsageFunc prints "..." for any flag that does not have a default value specified. This produces less-than-effective help from DefaultUsageFunc.

This change retains the behavior of printing the default value as-is, but if a default value is not provided, it allows users to provide placeholder text by wrapping a word inside the help text for a flag in backticks.

For example, given the following:

fset.String("c", "" /* default */, "path to `config` file")

We'll get:

-c config  path to config file

This matches the behavior of FlagSet.PrintDefaults, and indeed it relies on the same flag.UnquoteUsage machinery for this.

This also has the nice side-effect of making a reasonable guess at an alternative placeholder text instead of "...". For example:

fset.Int("n", "" /* default */, "number of items")

// Before: -n ...  number of items
// Now:    -n int  number of items

Note that as implemented right now, the user supplied placeholder will be used only if a non-zero default value was not supplied. This was an attempt to retain as much of the existing behavior. The proposal in #105, if you're open to it, would change more of the output.

peterbourgon commented 1 year ago

I'm open to allowing users to specify placeholder text other than ..., but I'm not convinced that text in backticks in the help string is a reasonable way to encode that information.

abhinav commented 1 year ago

RE: Prior art: this is how the std flag package does it. This is documented mostly in flag.PrintDefaults:

-x int
  usage-message-for-x (default 7)

[..] The listed type, here int, can be changed by placing a back-quoted name in the flag's usage string; the first such item in the message is taken to be a parameter name to show in the message and the back quotes are stripped from the message when displayed. For instance, given

flag.String("I", "", "search `directory` for include files")

the output will be

-I directory
  search directory for include files.

This PR isn't suggesting a completely custom behavior; just to carry over something that the std flag package already provides. I'll agree that this feature isn't super obviously documented in std flag, but users of flag who know about it will likely expect it from ffcli as well. (That's what brought me here! 😄 )

About whether this is the best way to encode this information: I have mixed feelings about it. While I like the idea of incorporating the variable name in the help message when possible, I would have preferred for std flag to have a more explicit means to set this when it's not. For example, by setting a field on flag.Flag, or implementing a method on a flag.Value. That might be worth pursuing with a proposal upstream, but I don't have the appetite for that right now.

So right now, this is just suggesting matching the behavior of what flag.PrintDefaults already provides.

peterbourgon commented 1 year ago

Oh, cool! Somehow I didn't know about that.

abhinav commented 1 year ago

@peterbourgon Obviously no rush, but FYI, the suggested change has been made.

peterbourgon commented 1 year ago

Thanks for the poke.