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: conventionalize on <command>
in cmd.Use
#7143
Conversation
8d797c3
to
dd63730
Compare
For other cases like
That's it. It should actually be the default in the template. Git for example, shows all the options in the usage. In our case I say only if there's a clear benefit in helping the user learn how to use the command, otherwise just leave the default. |
Ok 👌 👼 . Just to confirm, Run 'docker COMMAND --help' for more information on a command. We keep representing it as such: But, for the commands with a mandatory input such as So, to summarize, usage is now: dagger call [flags] [function]...
dagger call [flags] [command] with chained functions: dagger call base [flags]
dagger call base [flags] [command] 10. functions |
465bd9e
to
f2bdbf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update 04/23/2024
I added a new commit following Cobra's usage
Recommended syntax is as follows:
[ ] identifies an optional argument. Arguments that are not enclosed in brackets are required.
I’m not fully convinced on the second statement yet. It’s more common to use <>
as placeholder, see (POSIX) Utility Conventions, or at least put it in caps.
In dagger install module
how do you know if module
is something you have to type or replace? dagger install <module>
looks much clearer to me. If the angle brackets are just ugly, that’s why I actually prefer dagger install MODULE
, but as I’ve pointed out before, that may require more changes to go against Cobra’s default.
How about this, can you tell which is a command and which is an argument (assume only global flags exist here)?
dagger config views name set value
How about now?
dagger config views <name> set <value>
In the first one you rely on names that helps your brain fill in the blank, while in the second it’s more immediately obvious.
Lastly, I want to point out that adding [flags]
may help understand where the split is:
dagger run command
# vs
dagger run [flags] command
But then it may not apply in some case so I try to picture this without it:
dagger run command
# vs
dagger run <command>
But without differentiating mandatory and optional arguments, as proposed in your comment below: #7143 (comment).
That’s not what I suggested.
For other cases like
dagger config
, just follow whatdocker
does. Justdagger config [flags] [command]
. That is:
- if it has flags add
[flags]
- if it has sub-commands add
[command]
Ok 👌 👼 . Just to confirm,
docker --help
shows:Run 'docker COMMAND --help' for more information on a command.We keep representing it as such:
dagger [command] --help
This is correct, but find me a docker command that has subcommands and does something on its own. Most docker commands that have subcommands seem to be used only for grouping those subcommands, so not something you’d use directly. In that case it makes sense to mark the subcommand as required.
But dagger call
and dagger config
have valid usages without subcommands, thus the []
in [command]
.
But, for the commands with a mandatory input such as
dagger install [flags] MODULE
, how would you represent it ? ==> Is itdagger install [flags] module
? Or do we not make any distinction:dagger install [flags] [module]
Yes, we make the distinction.
To be clear, when I gave docker’s example it wasn’t to replicate the syntax or to forcibly adopt the “required” subcommand explicitly, it was to replicate the simplicity of delegating details to subcommands’ --help
, rather than be more verbose on the parent command’s usage.
You always need to take into account if you’re comparing apples to apples in regards to the syntax of different tools/clis.
cmd/dagger/call.go
Outdated
@@ -16,7 +16,7 @@ var outputPath string | |||
var jsonOutput bool | |||
|
|||
var callCmd = &FuncCommand{ | |||
Name: "call [flags] [FUNCTION]...", | |||
Name: "call [flags] [function]...", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will produce:
dagger call [flags] [function]...
dagger call [flags] [command]
Which isn't right because here, "function" and "command" are the same thing. As per the conventions:
When multiple synopsis lines are given for a utility, it is an indication that the utility has mutually-exclusive arguments.
This happens because you're describing function as a positional argument, when it's actually a subcommand. In the template, cobra sees that it has subcommands so it adds the second line.
Docker, for example, doesn't print two lines. It always prints the same if it has subcommands:
{{- if not .HasSubCommands}} {{.UseLine}}{{end}}
{{- if .HasSubCommands}} {{ .CommandPath}}{{- if .HasAvailableFlags}} [OPTIONS]{{end}} COMMAND{{end}}
In our case, we'd have to check if the command is required or not. That's a plus in docker for consistency that we don't have.
In this case the subcommand is indeed optional, but you mark that by making the usage about the call
command itself and not include children. If you change the usage to just call
and remove the square brackets in [command]
in the template, it produces this which is more correct:
dagger call [flags]
dagger call <command> [flags]
But Cobra's default is actually [command]
and in the cases where the command is actually required, then the first line doesn't make sense.
Since we have a mix (sometims required, sometimes optional), we may have to use a helper function to make a single usage line instead, that detects which applies. But I suggest keeping it for another PR and simply keep the usage here as:
Name: "call [flags] [function]...", | |
Name: "call", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that [flags]
was renamed to [options]
, we need to add it explicitly. As for the two usage lines in the template, I don't think we can generalize because we have a command that requires a subcommand, and another where it's optional. Better be explicit in cmd.Use
. I pushed an update with these changes so I can merge this quickly:
- Name: "call",
+ Name: "call [flags] [command]",
Notice that I'll rename [command]
to [function]
here in another PR.
f2bdbf3
to
1c3c1cc
Compare
@grouville, let me know when you're ready for another review 🙏 |
1c3c1cc
to
5e64783
Compare
ff2db86
to
532c6f9
Compare
Unifies the DX around usage across all commands. The convention follows the standards (gh / git CLIs) as well as the defaults from cobra. The grammar is as follow: - []: optional - <>: user input, when argument is not optional, to differenciate with static args - ...: one or more This commit applies it everywhere and unifies according to this grammar. Signed-off-by: grouville <guillaume@dagger.io>
<command>
in cmd.Use
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
532c6f9
to
cac69b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update to get this merged quickly
Sometimes a subcommand is required (`dagger call`), and other times it's optional (`dagger config`). Best make it explicit in `Use`. Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
cac69b6
to
0573d7c
Compare
* cli: conventionalize on <command> in cmd.Use Unifies the DX around usage across all commands. The convention follows the standards (gh / git CLIs) as well as the defaults from cobra. The grammar is as follow: - []: optional - <>: user input, when argument is not optional, to differenciate with static args - ...: one or more This commit applies it everywhere and unifies according to this grammar. Signed-off-by: grouville <guillaume@dagger.io> * Fix incorrections Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com> * Remove second use line Sometimes a subcommand is required (`dagger call`), and other times it's optional (`dagger config`). Best make it explicit in `Use`. Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com> * Fix required mark when flag has no description Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com> --------- Signed-off-by: grouville <guillaume@dagger.io> Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com> Co-authored-by: grouville <guillaume@dagger.io> Co-authored-by: Helder Correia <174525+helderco@users.noreply.github.com>
One of the action items discussed in #7107 (comment)
Unifies the DX around usage across all commands. The convention follows the standards (gh / git CLIs) as well as the defaults from cobra. More context from @helderco in above comment
Proposal
The grammar is as follow:
Another point: [flags] shall always be positioned directly after a command. Or, at least to have a consistent positionning across all commands.
Context
This PR applies the grammar everywhere for consistency. And, fixes the
dagger gen
anddagger config views
command where flags were not positioned in the same order as all the other commands.1. Open for debate / proposition
Both for
functions
andcall
, I took the liberty to modify the usage a bit to be more descriptive.The origin of this change is that I got suprised when testing
dagger functions
: I tested to chain two functions at the same level, which the previous usage could suggest:I am totally fine on keeping that format too, especially if you feel that it is easier to read and less confusing overall. You are the DX master 👼
The terminology could also be:
<child-function>
<derived-function>
<sub-function>
2. dagger config
It seems that the
dagger config
subcommand, which is still experimental and with hidden subcommands might need an issue on its own as the behavior of presenting sub-commands and the flags ordering seems a bit different.Update 04/23/2024
I added a new commit following Cobra's usage
But without differentiating mandatory and optional arguments, as proposed in your comment below: #7143 (comment). I also reverted the functions and the call usage as they were before
Update 05/02/2024
Following the discussion on the issue, proposed syntax:
Also, another inherent rule that you proposed: