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

cli: de-clutter CLI #7107

Closed
wants to merge 6 commits into from
Closed

cli: de-clutter CLI #7107

wants to merge 6 commits into from

Conversation

grouville
Copy link
Member

@grouville grouville commented Apr 17, 2024

Details of current draft implementation

@helderco we had a sync, but I might have forgotten some parts ahah (sorry 👼). Feel free to tell if I'm completely off on some interpretation / implementation:

  • Add line break between error and usage
    This has been added in cmd/dagger/functions.go

  • Style headings in BOLD UPPERCASE to help break sections visually
    A new helper function has been added and the corresponding template has been updated

  • Remove discrepancy between dagger query and the rest of the commands
    This sub-command hasn't been updated in months and having a separate template is more coordination work. It might be time to make them 1:1 ?

  • Conventionalize on either <command> or COMMAND syntax in cmd.Use
    I updated all COMMAND as uppercase. I did not remember if you were also referencing on the usage examples inside square brackets like [arguments] -> [ARGUMENTS] ? I followed the CLI examples we discussed and some did it that way, in doubt I applied it everywhere but totally open for change

  • Figure out if arguments are better above or below functions/commands ??
    I experimented on this one, and I haven't found an easy way: on a given function call, I do agree that having the commands at the bottom does make sense. However, changing the order on the template directly also changes the order for commands such as: dagger --help. What do you think is the best implementation on this one ? Shall we tweak the template to make it specific for just the dagger call section ? I feared adding complexity to the template logic

  • Visual cue for required flags
    As you proposed last time, the idea would be to have a required flags section for the dagger call function subcommands only ? Will take a look tomorrow. Same fear on the added complexity on the main template, but this one seems more manageable: it is just a section wrapped in an if to show it on dagger call subcommand context

  • "Options" instead of "Flags"?1
    This is done, with our own implementation of useLine to print options instead of args by default.
    Solomon requested arguments on above issue though. I followed your terminology, but shall we make a specific case for arguments in dagger call / dagger functions scope ?

  • Help topics for dagger call to keep --help lean while still providing more documentation
    What topics do you think we should cover first ? More than the implementation, the most important might be the documentation added to those topics. Which could be the first 2 topics to create, so that i draft the implementation tomorrow ?

  • Global flags only on root command and top level commands

    • Remove persistent flags from child commands after being parsed? Or just hide?
      • TUI (--silent, --focus)
      • Modules (-m)

This is the one I am currently trying to implement in a clean way (using the hiding method at first), and might need some guidance again. I have been trying to extend the callCmd implementation by implementing a new persistentPreRunE:

PersistentPreRunE: func(_ *FuncCommand, cmd *cobra.Command, _ []string) error {
    rootCmd := cmd
    for rootCmd.Parent() != nil {
        rootCmd = rootCmd.Parent()
    }
    hideGlobalFlags(rootCmd)
    return nil
},

to control the behavior and potentially use the funcCommand infos or the cmd infos to mark as hidden, as you suggested, when the call command has subcommands
However, I'm still having a hard time on this part. Will keep iterating tomorrow again

  • Talk about Functions and arguments instead of Function commands
    This is partially done as I used your options terminology globally instead of arguments. It is true that arguments is pretty accurate in a dagger call functions context, but then, do we talk about arguments inside dagger functions usage examples too ?

cc @helderco

grouville added 6 commits April 16, 2024 23:20
As recommended by Helder, visually break sections using bold uppercase for titles.

I also removed the special template for the query subcommand: it was used to put the example after the flags. However, this created a special UX just for this sub-command. We should better have a consistent positioning of examples prior args or after on all subcommands, so on the main template

Signed-off-by: grouville <guillaume@dagger.io>
When querying `dagger call --help` and `dagger functions --help` without being in a module, an error is being raised.
This commit adds a newline between the two, as it was hard to distinct between them

Signed-off-by: grouville <guillaume@dagger.io>
Remove colon from usage, as it is now bold and is redundent

Signed-off-by: grouville <guillaume@dagger.io>
…mands" and "flags"

1. Conventionalize on either <command> or COMMAND syntax in cmd.Use
2. Talk about "functions" and "arguments", rather than "function commands" and "flags"
3. "Options" instead of "Flags"? Solomon prefers arguments

Signed-off-by: grouville <guillaume@dagger.io>
Instead of Flags, rely on the option terminology. To comply with Solomon's request of arguments and functions, in the context of functions I kept Solomon's terminology: arguments, and for the rest it is options

Signed-off-by: grouville <guillaume@dagger.io>
1. Use OPTIONS everywhere instead of FLAGS
2. Overrides Cobra's default [flags] printing
3. Finish standardizing all options as uppercase

Signed-off-by: grouville <guillaume@dagger.io>
@grouville grouville requested a review from helderco April 17, 2024 07:21
@grouville grouville changed the title cli: de-clutter help cli: de-clutter CLI Apr 17, 2024
@helderco
Copy link
Contributor

helderco commented Apr 17, 2024

Fixes #6943 (partially for now, but foundation)

A separate PR for each item would actually be preferred, especially if it can stand on its own or it may warrant some discussion or design decision. In that case, don't forget to remove the "Fixes" here, so this doesn't close the issue until all PRs are marged.


  • Remove discrepancy between dagger query and the rest of the commands
    This sub-command hasn't been updated in months and having a separate template is more coordination work. It might be time to make them 1:1 ?

👍


  • Conventionalize on either <command> or COMMAND syntax in cmd.Use
    I updated all COMMAND as uppercase. I did not remember if you were also referencing on the usage examples inside square brackets like [arguments] -> [ARGUMENTS] ? I followed the CLI examples we discussed and some did it that way, in doubt I applied it everywhere but totally open for change

Yes, it's applied everywhere. The idea is to choose a syntax for representing usage and stick to it. Last time I changed to [OPTIONS] but than reverted back to [flags] because while I prefer the former for avoiding confusion with static argument names and user provided values in some cases, Cobra uses the latter syntax. So it can seem like swimming against the current because you need to override some default behavior. The Docker CLI does it, but I’m not sure it’s worth it for us.

The gh tool uses Cobra’s default, maybe copy that as it’s more common anyway, I believe. I suggest looking at git too since there’s lots of examples there.


  • Figure out if arguments are better above or below functions/commands ??
    I experimented on this one, and I haven't found an easy way: on a given function call, I do agree that having the commands at the bottom does make sense. However, changing the order on the template directly also changes the order for commands such as: dagger --help. What do you think is the best implementation on this one ? Shall we tweak the template to make it specific for just the dagger call section ? I feared adding complexity to the template logic

Cobra’s default is to put flags after subcommands. In some cases it seemed like I was looking for the flags next to the usage line, so I experimented with moving them there. That’s that docker does, but you’ll find a hard time finding a docker command with flags and sub-commands (example: docker compose --help).

However, when you have a lot of sub-commands, as when you do dagger call on a function that returns a container, then it’s much harder to find the flags because that long list will occupy the whole screen. Doubly confusing to have “Global flags” at the bottom since that “hints” that non-global flags would be next to them.

Note that with docker, in this case, you only see “Global options” in the root command (docker --help), not in any children. That inspired me to do the same.


  • Visual cue for required flags
    As you proposed last time, the idea would be to have a required flags section for the dagger call function subcommands only ? Will take a look tomorrow. Same fear on the added complexity on the main template, but this one seems more manageable: it is just a section wrapped in an if to show it on dagger call subcommand context

I wouldn’t put an if in the template looking for the call command specifically. In this case it can be more general. Rather than breaking in two sections because of required, just append [required] to flags marked as required, same as what’s done for default values, and make sure they’re sorted before the others (better in its own PR).

It’s what Typer does, for example:

Usage: app [OPTIONS] NAME  

Options:  
  --lastname TEXT       Last name of the person [required]
  --help                Show this message and exit.

The idea of a “required option” can be strange though, but at least right now that only happens in dagger call, which brings us to the next point.


  • "Options" instead of "Flags"?1
    This is done, with our own implementation of useLine to print options instead of args by default.
    Solomon requested arguments on above issue though. I followed your terminology, but shall we make a specific case for arguments in dagger call / dagger functions scope ?

dagger functions doesn’t show any function arguments. It doesn’t even create sub-commands so they don’t show in --help as well. It works by normal stdout so no customization necessary here.

Also, remember that dagger call (top level command) is the only command that can have two kinds of flags. An actual flag like --output and flags from constructor arguments. But for any dagger call <function> sub-command, it’s just function arguments.

If it was just the latter, it would be simply a matter of customizing the name of the section via an annotation, for example. But I think there’s a value in separating the flags in call and the constructor arguments.

How to do that? If you see how marking a flag as required works, Cobra is adding an annotation through the flagset. So you can just create an annotation for marking a flag as a “function argument”, and in the template, similarly to how you handle required flags, you split argument flags, and if there’s any, show a section below “Options” for “Arguments”.

Both sections will only show at the same time in the top-level dagger call and flags marked as “required" will only show under Arguments so there’s no “required option” incongruity in practice.

I suggest three separate PRs then:

  1. Replace “Flags” with “Options”
  2. Append [required] to a flag marked as required, and sort the list of flags with the required ones first
  3. Adopt “Functions” and “Arguments” terminology in dagger call

  • Help topics for dagger call to keep --help lean while still providing more documentation
    What topics do you think we should cover first ? More than the implementation, the most important might be the documentation added to those topics. Which could be the first 2 topics to create, so that i draft the implementation tomorrow ?

Not a priority, better create a separate issue for this. The intent is to provide more information to the user in the CLI (i.e., without requiring a visit to the docs), while reducing the clutter in normal --help. A good example for dagger call is how it works through chaining, what happens with a few core types (Contaienr, Directory…), how to use --output and --json, etc.

gh uses help topics for this, but docker relies on web documentation, doing its utmost to keep --help output to a minimum. I didn’t find any help topic being used (as a feature) in that CLI.

Since Dagger has its nuances, I’ve been wondering which approach is better here. We could just “link” to the web for more information. \cc @vikram-dagger


  • Global flags only on root command and top level commands

    • Remove persistent flags from child commands after being parsed? Or just hide?

      • TUI (--silent, --focus)
      • Modules (-m)

This is the one I am currently trying to implement in a clean way (using the hiding method at first), and might need some guidance again. I have been trying to extend the callCmd implementation by implementing a new persistentPreRunE:

PersistentPreRunE: func(_ *FuncCommand, cmd *cobra.Command, _ []string) error {
    rootCmd := cmd
    for rootCmd.Parent() != nil {
        rootCmd = rootCmd.Parent()
    }
    hideGlobalFlags(rootCmd)
    return nil
},

to control the behavior and potentially use the funcCommand infos or the cmd infos to mark as hidden, as you suggested, when the call command has subcommands However, I'm still having a hard time on this part. Will keep iterating tomorrow again

The way I’ve envisioned this is through main.go, for all commands. Not just call. docker hides them for any command that’s not the root command. So you’d only see --debug and --silent for example, in dagger --help but not dagger call.

I think some of those can be useful at “top level” though (direct child of root), especially -m,--mod, but has been painful only in dagger call <function> sub-commands actually. Additionally, some of the global flags have already been either hidden or removed since I first added this to the item list.

So I’d just not add these flagsets to “function” sub-commands in call (not simply hide), which also helps prevent collisions with a user’s argument name choices. Including -m,--mod


  • Talk about Functions and arguments instead of Function commands
    This is partially done as I used your options terminology globally instead of arguments. It is true that arguments is pretty accurate in a dagger call functions context, but then, do we talk about arguments inside dagger functions usage examples too ?

Already talked about this in a previous point. To reiterate, "Options" is also closer to function arguments than “Flags”, but it’s better to have constructor arguments in dagger call be separate from that command’s own flags like --output, and it also avoids having “required options”, since that only happens for function arguments.

None of this applies to dagger functions since it works differently. It’s not through --help like call. There’s no child subcommands and not flags from function arguments, just normal positional arguments to the functions command.

@helderco
Copy link
Contributor

helderco commented Apr 19, 2024

@grouville, with "Figure out if arguments are better above or below functions/commands", I realize I wasn't clear in addressing your question. The answer to doing it only for call is no. Do it for every command, i.e., basically revert this commit: a8ae912

Note that only dagger call may have a long list of sub-commands. But if there's a command (like dagger config) that can also have sub-commands, it won't be that big, so I don't think you'll notice that much if the flags are below or above the sub-commands as they should be easy to find. So repositioning for all commands is the best option.

grouville pushed a commit to grouville/dagger that referenced this pull request Apr 23, 2024
1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)

Signed-off-by: grouville <guillaume@dagger.io>
grouville pushed a commit to grouville/dagger that referenced this pull request Apr 23, 2024
1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)

Signed-off-by: grouville <guillaume@dagger.io>
grouville pushed a commit to grouville/dagger that referenced this pull request Apr 23, 2024
1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)
3. Set DisableFlagsInUseLine to true for AppendedSubcommands in dagger call

Signed-off-by: grouville <guillaume@dagger.io>
grouville pushed a commit to grouville/dagger that referenced this pull request Apr 24, 2024
1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)
3. Set DisableFlagsInUseLine to true for AppendedSubcommands in dagger call

Signed-off-by: grouville <guillaume@dagger.io>
grouville added a commit that referenced this pull request Apr 25, 2024
* cli: rename flags to options

1. Rename all the flag mentions to "options", as proposed by Helder in #7107 (comment)
2. Remove the automatic [flags] append on all commands: #7107 (comment)
3. Set DisableFlagsInUseLine to true for AppendedSubcommands in dagger call

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix usage of watch command

- Remove the COMMAND copy-paste error, where a command is not required for the watch command
- Do not show the [options], as only inherited flags are shown

Signed-off-by: grouville <guillaume@dagger.io>

* cli: remove [options] for dagger version

Do not show [options] for command with only inherited flags

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix and add <pattern>... to config views add / set / remove commands

Applies the recommendations from Helder on those sub commands

Signed-off-by: grouville <guillaume@dagger.io>

---------

Signed-off-by: grouville <guillaume@dagger.io>
vikram-dagger pushed a commit to vikram-dagger/dagger that referenced this pull request May 3, 2024
* cli: rename flags to options

1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)
3. Set DisableFlagsInUseLine to true for AppendedSubcommands in dagger call

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix usage of watch command

- Remove the COMMAND copy-paste error, where a command is not required for the watch command
- Do not show the [options], as only inherited flags are shown

Signed-off-by: grouville <guillaume@dagger.io>

* cli: remove [options] for dagger version

Do not show [options] for command with only inherited flags

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix and add <pattern>... to config views add / set / remove commands

Applies the recommendations from Helder on those sub commands

Signed-off-by: grouville <guillaume@dagger.io>

---------

Signed-off-by: grouville <guillaume@dagger.io>
vikram-dagger pushed a commit to vikram-dagger/dagger that referenced this pull request May 3, 2024
* cli: rename flags to options

1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)
3. Set DisableFlagsInUseLine to true for AppendedSubcommands in dagger call

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix usage of watch command

- Remove the COMMAND copy-paste error, where a command is not required for the watch command
- Do not show the [options], as only inherited flags are shown

Signed-off-by: grouville <guillaume@dagger.io>

* cli: remove [options] for dagger version

Do not show [options] for command with only inherited flags

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix and add <pattern>... to config views add / set / remove commands

Applies the recommendations from Helder on those sub commands

Signed-off-by: grouville <guillaume@dagger.io>

---------

Signed-off-by: grouville <guillaume@dagger.io>
vikram-dagger pushed a commit to vikram-dagger/dagger that referenced this pull request May 3, 2024
* cli: rename flags to options

1. Rename all the flag mentions to "options", as proposed by Helder in dagger#7107 (comment)
2. Remove the automatic [flags] append on all commands: dagger#7107 (comment)
3. Set DisableFlagsInUseLine to true for AppendedSubcommands in dagger call

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix usage of watch command

- Remove the COMMAND copy-paste error, where a command is not required for the watch command
- Do not show the [options], as only inherited flags are shown

Signed-off-by: grouville <guillaume@dagger.io>

* cli: remove [options] for dagger version

Do not show [options] for command with only inherited flags

Signed-off-by: grouville <guillaume@dagger.io>

* cli: fix and add <pattern>... to config views add / set / remove commands

Applies the recommendations from Helder on those sub commands

Signed-off-by: grouville <guillaume@dagger.io>

---------

Signed-off-by: grouville <guillaume@dagger.io>
Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label May 18, 2024
@grouville
Copy link
Member Author

grouville commented May 20, 2024

Was split in several sub-PRs, and Helder is now continuing. Can be closed, as not relevant anymore

@grouville grouville closed this May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants