Closed ChrisHines closed 1 month ago
My prototype is in #73. I have submitted it as a Draft since it is merely a suggestion at this point and I have made no effort to update any documentation to reflect the API changes.
Oops, I saw the issue after I saw the PR. To echo my response from there: this seems easily solved by using fully-qualified ShortUsage strings for each subcommand, e.g.
barfs := flag.NewFlagSet("bar", flag.ContinueOnError)
barfs.String("bf", "", "bar flag")
bar := &ffcli.Command{
Name: "bar",
FlagSet: barfs,
ShortUsage: "root [flags] foo [flags] bar [flags] <arg> [<arg> ...]",
}
foofs := flag.NewFlagSet("foo", flag.ContinueOnError)
foofs.String("ff", "", "foo flag")
foo := &ffcli.Command{
Name: "foo",
FlagSet: foofs,
ShortUsage: "root [flags] foo [flags] <subcommand>",
Subcommands: []*ffcli.Command{bar},
}
rootfs := flag.NewFlagSet("root", flag.ContinueOnError)
rootfs.String("rf", "", "root flag")
root := &ffcli.Command{
Name: "root",
FlagSet: rootfs,
ShortUsage: "root [flags] <subcommand>",
Subcommands: []*ffcli.Command{foo},
}
This also feels like the most context from parent commands that I'd actually want to see in a given subcommand. That is, in your example, I explicitly don't want to see the flagset definitions from the parents — it feels superfluous, or noisy. (In the case of "global" flags that are valid in all subcommands, the pattern is to include/duplicate them in each subcommand's FlagSet, so they would already be visible.)
Maybe I'm missing a use case... please enlighten me! :)
this seems easily solved by using fully-qualified ShortUsage strings for each subcommand
Indeed, that is a solution, but as a programmer I would rather not have to couple subcommand metadata to its position in the hierarchy or to repeat myself in that fashion, especially as the set of subcommands grows. That has proven fragile in my experience and a colleague I spoke with recently shared that experience. I would prefer the ability to write a UsageFunc
that pulls the information from the other Command
fields in a consistent way so I don't have to manually curate ShortUsage
strings.
I explicitly don't want to see the flagset definitions from the parents — it feels superfluous, or noisy.
Fair enough. I disagree, but that's OK and I can also foresee the choice differing from app to app based on whatever UX forces are in play. I would be happy if we left the ffcli default behavior as it is and only change the UsageFunc
API in a way to allow access to the full command path from root to terminal. That would let me or others write custom UsageFunc
s that provide the level of detail necessary for our projects. My draft PR that changed DefaultUsageFunc
is primarily a way of demonstrating the possibilities that such an API change would enable.
In the case of "global" flags that are valid in all subcommands, the pattern is to include/duplicate them in each subcommand's FlagSet, so they would already be visible.
I understand this pattern from the objectctl
example, but it is certainly not the only way we can use the exported API to construct a command tree. Furthermore, all other evidence in the package code and docs seems to contradict this advice. Both the textctl
example (which is duplicated in the README) and ExampleCommand_Parse_then_Run
contradict this advice and both are more prominently displayed in the documentation than objectctl
. More subtle, but no less prominent, the ShortUsage
field docs suggest non-leaf commands that accept flags are a common use case.
// ShortUsage string for this command. Consumed by the DefaultUsageFunc and
// printed at the top of the help output. Recommended but not required.
// Should be one line of the form
//
// cmd [flags] subcmd [flags] <required> [<optional> ...]
//
// If it's not provided, the DefaultUsageFunc will use Name instead.
// Optional, but recommended.
ShortUsage string
So I and others mostly see a different pattern when learning about the package.
I could be persuaded that only leaf commands should have a non-nil flag set. That seems like a valid design choice that guides us toward more ergonomic CLIs, but I don't claim to be an expert in that realm. If experience with building CLI's with ffcli has persuaded you that is the best approach then at a minimum we should update the docs to reflect that advice. Beyond that, if you think ffcli should not support flags on non-leaf commands perhaps a more wholistic rethink of the exported API toward a design that enforces (and helps automate) that policy is in order.
But the current API, docs, and parsing behavior clearly do support flags on non-leaf commands well, with the exception of not passing enough context to UsageFunc
s, and that's primarily what I think needs addressing.
I understand the points you're making, but I'm having trouble evaluating them. Can you give me some kind of concrete example? Various -h
outputs from a [theoretical] command where this feature would be especially important?
I could be persuaded that only leaf commands should have a non-nil flag set.
I didn't mean to suggest that; it's important to me that e.g. objectctl -v update -xyx ...
can work, and you don't have to shuttle all the flags to the end. But in order for that to work, for -v
to work after either the root objectctl
command, or the update
subcommand, you have to put a v
flag in both of those commands' flagsets. That's unavoidable (at the moment) and has the (nice) side effect of objectctl update -h
enumerating all possible flags.
Thanks for clarifying the design goals.
When a global flag is duplicated in all subcommands how do you typically indicate to the end user which flags are global and which are specific to the subcommand? Maybe they don't care most of the time, but I can imagine some confusion and frustration if they see -v
work after the root command and then other flags fail when they put them in that position.
Even so, if UsageFunc
had access to each level of Command
it could be written to de-dup flags and present them in a fashion appropriate to the project's UX goals. For example it could present them in separate sections as I showed above, or it could present them in a single list but annotate the global flags in some fashion. It could also be written to ignore all but the terminal command for behavior equivalent to the current implementation.
For a concrete example we can use the fly
CLI from https://concourse-ci.org. I am not involved in the implementation but it comes to minds since I have been learning it recently and it has a large CLI surface area. Here are a couple examples of it's help docs:
$ fly -h
Usage:
fly [OPTIONS] <command>
Application Options:
-t, --target= Concourse target name
-v, --version Print the version of Fly and exit
--verbose Print API requests and responses
--print-table-headers Print table headers even for redirected output
Help Options:
-h, --help Show this help message
Available commands:
abort-build Abort a build (aliases: ab)
active-users List the active users since a date or for the past 2 months (aliases: au)
archive-pipeline Archive a pipeline (aliases: ap)
builds List builds data (aliases: bs)
check-resource Check a resource (aliases: cr)
check-resource-type Check a resource-type (aliases: crt)
checklist Print a Checkfile of the given pipeline (aliases: cl)
clear-task-cache Clears cache from a task container (aliases: ctc)
completion generate shell completion code
containers Print the active containers (aliases: cs)
curl curl the api (aliases: c)
delete-target Delete target (aliases: dtg)
destroy-pipeline Destroy a pipeline (aliases: dp)
destroy-team Destroy a team and delete all of its data (aliases: dt)
disable-resource-version Disable a version of a resource (aliases: drv)
edit-target Edit a target (aliases: etg)
enable-resource-version Enable a version of a resource (aliases: erv)
execute Execute a one-off build using local bits (aliases: e)
expose-pipeline Make a pipeline publicly viewable (aliases: ep)
format-pipeline Format a pipeline config (aliases: fp)
get-pipeline Get a pipeline's current configuration (aliases: gp)
get-team Show team configuration (aliases: gt)
help Print this help message
hide-pipeline Hide a pipeline from the public (aliases: hp)
hijack Execute a command in a container (aliases: intercept, i)
jobs List the jobs in the pipelines (aliases: js)
land-worker Land a worker (aliases: lw)
login Authenticate with the target (aliases: l)
logout Release authentication with the target (aliases: o)
order-pipelines Orders pipelines (aliases: op)
pause-job Pause a job (aliases: pj)
pause-pipeline Pause a pipeline (aliases: pp)
pin-resource Pin a version to a resource (aliases: pr)
pipelines List the configured pipelines (aliases: ps)
prune-worker Prune a stalled, landing, landed, or retiring worker (aliases: pw)
rename-pipeline Rename a pipeline (aliases: rp)
rename-team Rename a team (aliases: rt)
rerun-build Rerun a build (aliases: rb)
resource-versions List the versions of a resource (aliases: rvs)
resources List the resources in the pipeline (aliases: rs)
schedule-job Request the scheduler to run for a job. Introduced as a recovery command for the v6.0 scheduler. (aliases: sj)
set-pipeline Create or update a pipeline's configuration (aliases: sp)
set-team Create or modify a team to have the given credentials (aliases: st)
status Login status
sync Download and replace the current fly from the target (aliases: s)
targets List saved targets (aliases: ts)
teams List the configured teams (aliases: t)
trigger-job Start a job in a pipeline (aliases: tj)
unpause-job Unpause a job (aliases: uj)
unpause-pipeline Un-pause a pipeline (aliases: up)
unpin-resource Unpin a resource (aliases: ur)
userinfo User information
validate-pipeline Validate a pipeline config (aliases: vp)
volumes List the active volumes (aliases: vs)
watch Stream a build's output (aliases: w)
workers List the registered workers (aliases: ws)
$ fly builds -h
Usage:
fly [OPTIONS] builds [builds-OPTIONS]
Application Options:
-t, --target= Concourse target name
-v, --version Print the version of Fly and exit
--verbose Print API requests and responses
--print-table-headers Print table headers even for redirected output
Help Options:
-h, --help Show this help message
[builds command options]
-a, --all-teams Show builds for the all teams that user has access to
-c, --count= Number of builds you want to limit the return to (default: 50)
--current-team Show builds for the currently targeted team
-j, --job=PIPELINE/JOB Name of a job to get builds for
--json Print command result as JSON
-p, --pipeline= Name of a pipeline to get builds for
-n, --team= Show builds for these teams
--since= Start of the range to filter builds
--until= End of the range to filter builds
Just a note that I'm still processing this. Thanks for the feedback.
I hope this will be addressed by #113.
Thanks. I'll have to explore that at some point. I've been making more use of these packages lately, although the use cases have been simpler than the one I presented in this issue. I have been writing some usage functions in those scenarios to help produce good usage text. I'll try to find some time to see if v4
would eliminate some of my custom code or if I have any suggestions.
Thank you. I'm happy to refactor the API based on user need.
I've just completed updating the project that inspired this issue from using the v3
API (with my forked changes) to using the v4-alpha.4
API unmodified.
In the intervening years I've come around to the idea that the user should provide all flags (even global ones) after the subcommands. But as a programmer I still wanted it to be easy to define those global flags once and reuse them for all subcommands. FlagSet.SetParent
method in v4 seems to solve that problem nicely.
I also appreciate the effort to split out the ffhelp
package and make that a separate concern. FWIW, although ffhelp.Command
doesn't produce exactly the help format that I was with my forked v3, it is plenty good enough for my needs and I am using it directly now.
I have settled on some code patterns using this package that I think strike a great balance between explicitness, conciseness, and readability and v4's help output is quite nice without too much fuss. Thanks.
I am satisfied enough to close this issue.
Run the program below (or on the playground) to see an example of the problem.
It prints:
It should be obvious that the above help information is inadequate. The bar command is presented detached from its context as a subcommand of
foo
which is likewise a subcommand ofroot
.I suggest that the help information should look more like this:
We can write a custom
UsageFunc
to produce that help text, but that doesn't scale, is tedious to maintain, and couples subcommand metadata tightly to its position in the command tree. I suggest thatffcli
should be able to create the above help text from only the metadata provided in the program shown above.I have prototyped a change that achieves that goal and will submit a PR showing that work for discussion. Note: The change requires a breaking API change.