-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update command guidelines #5769
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/5769 |
59e0e50
to
c638679
Compare
doc/command_guidelines.md
Outdated
|
||
- (*) Don't use single word verbs if they could cause confusion with the standard command types. For example, don't use `get` or `new` as these sound functionally the same as `show` and `create` respectively, leading to confusion as to what the expected behavior should be. | ||
e.g. `new`, `get` | ||
- (*) Descriptive, hyphenated command names are often a better option than single verbs. |
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.
The use of (*)
was a confusing to me at first--I thought it was mangled markup. Then I scrolled back to the top and saw. You might simply call this out in-text:
"Blah blah blah, don't use new (modules only)"
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 do call it out at the top so now that you know what the (*)
means, does it make sense?
I didn't want to prefix each of those lines with (command modules only)
as it's kind of verbose.
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.
Assuming everyone reads the entire document from the top, and pays attention, then yes. Many people I think will simply scroll to where ever they think the info they want is, especially if they follow a link to a particular section. Since it's a help document, being verbose is probably better than being cryptic, but I am fine if you want to leave it as is.
doc/command_guidelines.md
Outdated
For commands that don't conform to one of the above-listed standard command patterns, use the following guidance. | ||
|
||
- (*) Don't use single word verbs if they could cause confusion with the standard command types. For example, don't use `get` or `new` as these sound functionally the same as `show` and `create` respectively, leading to confusion as to what the expected behavior should be. | ||
e.g. `new`, `get` |
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.
Why is this line here? It's already mentioned in the text.
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.
removed.
doc/command_guidelines.md
Outdated
- If a command subgroup would only have a single command, move it into the parent command group and hyphenate the name. This is common for commands which exist only to pull down cataloging information. | ||
e.g. `database list-sku-definitions` instead of `database sku-definitions list` | ||
- Avoid command subgroups that have no commands. This often happens at the first level of a command branch. | ||
e.g. `keyvault create` instead of `keyvault vault create` (where `keyvault` only has subgroups and adds unnecessary depth to the tree). |
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 prefer the original, even though it is longer. The example would likely not be helpful at all to someone who isn't us without the preserved help output. When I read this, it sounds like it's keyvault specific and misses the bigger picture.
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.
added to an expandable section.
doc/command_guidelines.md
Outdated
- Provide custom table outputs for commands that don't provide table output automatically | ||
- Commands must return an object or dictionary (do not string, Boolean, etc. types) | ||
- Use `stdout` and `stderr` appropriately. | ||
- Command output must go to stdout, everything else to stderr (log/status/errors). |
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.
Since this line defines what "appropriately" is for stdout and stderr, does line 20 need to be said?
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.
removed that line.
doc/command_guidelines.md
Outdated
- You must provide command examples for non-trivial commands | ||
- All commands must support all output types (json, tsv, table) | ||
- Provide custom table outputs for commands that don't provide table output automatically | ||
- Commands must return an object or dictionary (do not string, Boolean, etc. types) |
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 makes it sounds like all commands must return something, which isn't the case.
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've clarified that None
can be returned also.
doc/command_guidelines.md
Outdated
- For nouns that only support a single verb, the command should be named as a single hyphenated verb-noun pair | ||
- All commands, command group and arguments must have descriptions | ||
- You must provide command examples for non-trivial commands | ||
- All commands must support all output types (json, tsv, table) |
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 isn't strictly true. There are some convenience commands that output in JSON only regardless of format, or the ACS command that outputs YAML.
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.
Those are exceptions that aren't ideal so would rather not call that out as part of the guidelines doc which should be recommendations on what you should be doing.
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.
Agreed. Perhaps then change "must" to "should" :)
doc/command_guidelines.md
Outdated
- Be consistent with POSIX tools (support piping, work with grep, awk, jq, etc.) | ||
- Support tab completion for parameter names and values (e.g. resource names) | ||
- Commands must follow a "[noun] [noun] [verb]" pattern | ||
- For nouns that only support a single verb, the command should be named as a single hyphenated verb-noun pair |
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 is stated later on.
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.
removed
doc/command_guidelines.md
Outdated
|
||
- Be consistent with POSIX tools (support piping, work with grep, awk, jq, etc.) | ||
- Support tab completion for parameter names and values (e.g. resource names) | ||
- Commands must follow a "[noun] [noun] [verb]" pattern |
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.
Consider moving this down into the "Command Naming and Behavior Guidance" section.
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.
done
|
||
If in doubt, ask! | ||
|
||
## General Patterns |
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.
It seems like some of the stuff in General Patterns would better be placed under other subheadings. I call out a few but best to consider them all.
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 think a separation between 'general patterns' and other things that pertain only to 'command naming' etc. makes sense.
Arguably everything could be put under 'Behavior' but keeping each section a few points each makes things clearer I think.
format fix md
c638679
to
8ce2ec7
Compare
* Update command guidelines format fix md * Address feedback
No description provided.