new HelpOption: Allow full subcommand flag display

[keilin-anz]

Problem Statement

By default, only required flags are displayed for subcommands

For applications with a lot of simple subcommands this obscures basic usage flags

Example (click to expand)

NB: this is an entirely contrived example, but is loosely based on my real world use case

Before (and still the default behaviour)

For a user to discover that there are optional flags on these subcommands they'd have to know to look

Usage: mycommand <command>

Flags:
  -h, --help                       Show context-sensitive help.

Commands:
  resource create
    Create a resource

  resource delete <name>
    Delete a resource

After (and entirely optional)

Now the user can see that resource delete has an optional flag without knowing to look for it

Usage: mycommand <command>

Flags:
  -h, --help                       Show context-sensitive help.

Commands:
  resource create
    Create a resource

  resource delete [--labels=KEY=VALUE;...] <name> 
    Delete a resource

Approach taken

• Add a new HelpOption called SubcommandsWithOptionalFlags

• Move Node.Summary() functionality into Node.SummaryWithOptions(bool)

  • Retain Node.Summary() as a default usage (forwards default
    value to Node.SummaryWithOptions(bool))

• Adds Parent field to Value, allowing us to easily check whether
  a value originated with a given Node

  • Modify build.go@buildField() to include Node Parent

• Modify help.go@printCommandSummary() to forward the
  HelpOption to Node.SummaryWithOptions() instead of
  calling Node.Summary()

• When the option is enabled:

  • Groups required flags
  • Groups optional flags (by checking if the flag originated on
    the current Node)
  • Prints required flags followed by optional flags surrounded
    by brackets, eg.
    --mandatory [ --optional1 --optional2 ]

• Removes broken logic from previous commit checking if a flag is named "help"

  • Now just checks if the flag belongs to the current Node

• Added criteria to the test suite to confirm:

  • Default behaviour is as before
  • Ancestor's optional flags are not included
  • Ancestor's required flags are included

Caveats

The main issue with this implementation is due to the Node receiver
functions doing display logic internally. This makes sense
for most of the use cases (application error logic), but
overly restricts the help configurability

As a result:

• Must pass awkward bool value down from the help context
• FlagSummary() ends up having to explicitly exclude the help flag - this is particularly janky (resolved earlier)

Potential Improvements to be made

• Remove explicit exclusion of help from FlagSummary()
  • Potentially add an equivalent Node receiver which just returns an
    array of flag summaries to be displayed by help@printCommandSummary
  • Then it's up to the help writer to decide how to render flags, and which flags to exclude when in the help context
• Change to FlagSummary()'s arity is a breaking change and unacceptable, seek alternative
  • See original comment
  • Updated to instead add a few duplicated lines whilst avoiding changing existing function arity

[keilin-anz]

@alecthomas - this is the change as little as possible version, happy to do a pass which removes a little magic (but would remote it broader changes)

Also not super convinced about the new HelpOption's name, open to suggestions - perhaps SubcommandsShowOptionalFlags?

[keilin-anz]

Converting to draft as it probably needs to (optionally) filter inherited flags, can get super noisy!

This would also fix the annoying explicit help exclusion

[keilin-anz]

Much improved now

Example output:

$ go run ./_examples/shell/optionalflags

Usage: help --mandatory <command>

An app demonstrating SubcommandsWithOptionalFlags

Flags:
  -h, --help              Show context-sensitive help.
      --dry-run=STRING    optional dry run flag
      --mandatory         mandatory global flag

Commands:
  resource create --mandatory --name=STRING
    create a resource

  resource delete --mandatory --scope=STRING [--labels=LABELS,... --version=STRING]
    delete resource(s)

Run "help <command> --help" for more information on a command.

[keilin-anz]

@alecthomas happy to create an issue against this if desired, just say the word!

[alecthomas]

Sorry for the epically late response. Overall the change looks good, but I can't merge this as it's a breaking API change.

[keilin-anz]

Sorry for the epically late response

All good! my turn this time, was on a short break :)

I can't merge this as it's a breaking API change.

FlagSummary is a fairly short function - so if you don't mind my duplicating it I could turn this into an additive change, ie.

• Additive changes:
  • add Node.FlagSummaryWithOptions(bool, bool)
  • Node.SummaryWithOptions() calls Node.FlagSummaryWithOptions(bool, bool) instead
• Reverted:
  • Node.FlagSummary() returns to its original arity
  • Node.Summary() goes back to calling Node.FlagSummary(bool)

[keilin-anz]

Actually - given the current PR is unacceptable, I'll just make the changes now and see what you think :)

[keilin-anz]

Rebased, reworked changes to avoid API modification (and attempting to minimise duplication where possible)

[keilin-anz]

Just extra explanation re. bonus change in b196f4d : appears that my rebase picked up some change which is invalid with the current golangci-lint (see previous execution here: https://github.com/alecthomas/kong/actions/runs/3126674306/jobs/5072442276)

Seem fine now

[keilin-anz]

Rebased, reverted the removal of nodupl lint rules