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

ffcli.DefaultUsageFunc: Support flag help placeholders #106

Merged
merged 1 commit into from May 9, 2023

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented May 3, 2023

(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
Copy link
Owner

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.

//
// Otherwise, it's an educated guess for a placeholder,
// or an empty string if one couldn't be determined.
argname, usage := flag.UnquoteUsage(f)
Copy link
Owner

Choose a reason for hiding this comment

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

This appears to be the major change, is there some prior art that uses UnquoteUsage in this way?

Copy link
Owner

Choose a reason for hiding this comment

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

Apparently flag.FlagSet lol 😮‍💨

@abhinav
Copy link
Contributor Author

abhinav commented May 6, 2023

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
Copy link
Owner

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

ffcli/command.go Outdated Show resolved Hide resolved
(This is an arguably uncontroversial part of the proposal in peterbourgon#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 peterbourgon#105, if you're open to it,
would change more of the output.
@abhinav
Copy link
Contributor Author

abhinav commented May 8, 2023

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

@peterbourgon
Copy link
Owner

Thanks for the poke.

@peterbourgon peterbourgon merged commit 61fa353 into peterbourgon:main May 9, 2023
@abhinav abhinav deleted the flag-placeholder branch May 9, 2023 13:50
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

Successfully merging this pull request may close these issues.

None yet

2 participants