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
cmd,dev-cmd: indicate multiple named args in usage banner #10337
Conversation
Review period will end on 2021-01-18 at 12:09:22 UTC. |
Review period skipped due to |
Marking as critical as we have failing[1] workflow runs now and this PR contains fixes. [1] https://github.com/Homebrew/homebrew-core/runs/1708280747?check_suite_focus=true |
seems like this command takes named args somewhere
Instead of having repeated items in the usage banner, use the following convention:
|
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 really appreciate you going through and checking this but: this definitely feels like the type of thing that should be programmatically generated. I don't think that should block this PR being merged (as it's an improvement) but this top line feels like something we can infer from everything else we know about a command (so should do so).
This is nice. |
I also thought about generating those automatically, but for now I went this route. Regarding this style suggestion made by Eric, I adopted the current style from |
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 awesome, thanks for looking at this @dawidd6!
I have just a few comments, but nothing too serious to need immediate action or anything.
And thanks for picking up on the pr-*
issues!
named_args :formula | ||
named_args :formula, min: 1 |
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 intentionally didn't add a minimum here because of the error message text. With the minimum, here are some sample cases:
$ brew desc
Usage: brew desc [options] text|/text/|formula [text|/text/|formula
...]
...
Error: Invalid usage: this command requires a formula argument
$ brew desc --search
Usage: brew desc [options] text|/text/|formula [text|/text/|formula
...]
...
Error: Invalid usage: this command requires a formula argument
This is fine, but in the second case, the missing argument isn't really a formulae argument. Is there a specific reason you changed? I'm happy to keep it as-is if so, but otherwise, I think I'd rather it be changed bask (which I will gladly do)
@@ -17,7 +17,7 @@ module Homebrew | |||
def desc_args | |||
Homebrew::CLI::Parser.new do | |||
usage_banner <<~EOS | |||
`desc` [<options>] (<text>|`/`<text>`/`|<formula>) | |||
`desc` [<options>] <text>|`/`<text>`/`|<formula> [<text>|`/`<text>`/`|<formula> ...] |
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.
FYI this new usage banner is a little too long:
$ brew help desc
Usage: brew desc [options] text|/text/|formula [text|/text/|formula
...]
Display formula's name and one-line description. Formula descriptions are
cached; the cache is created on the first search, making that search slower than
subsequent ones.
-s, --search Search both names and descriptions for
text. If text is flanked by slashes, it
is interpreted as a regular expression.
-n, --name Search just names for text. If text is
flanked by slashes, it is interpreted as a
regular expression.
-d, --description Search just descriptions for text. If
text is flanked by slashes, it is
interpreted as a regular expression.
--debug Display any debugging information.
-q, --quiet Make some output more quiet.
-v, --verbose Make some output more verbose.
-h, --help Show this message.
We'll probably want to shorten it in the future so it fits on one line. Also probably worth having a rubocop to check for this (if it's not automatically generated)
@@ -19,7 +19,7 @@ def switch_args | |||
Symlink all of the specified <version> of <formula>'s installation into Homebrew's prefix. | |||
EOS | |||
|
|||
named 2 | |||
named_args number: 2 |
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.
Fair enough. I didn't bother because it's disabled and will be removed, but no harm. Same for brew diy
@@ -15,7 +15,7 @@ module Homebrew | |||
def style_args | |||
Homebrew::CLI::Parser.new do | |||
usage_banner <<~EOS | |||
`style` [<options>] [<file>|<tap>|<formula>] | |||
`style` [<options>] [<file>|<tap>|<formula>|<cask>] [<file>|<tap>|<formula>|<cask> ...] |
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.
Another really long line:
$ brew help style
Usage: brew style [options] [file|tap|formula|cask]
[file|tap|formula|cask ...]
Check formulae or files for conformance to Homebrew style guidelines.
Lists of file, tap and formula may not be combined. If none are provided,
style will run style checks on the whole Homebrew library, including core code
and all formulae.
--fix Fix style violations automatically using
RuboCop's auto-correct feature.
--display-cop-names Include the RuboCop cop name for each
violation in the output.
--reset-cache Reset the RuboCop cache.
--formula, --formulae Treat all named arguments as formulae.
--cask, --casks Treat all named arguments as casks.
--only-cops Specify a comma-separated cops list to
check for violations of only the listed
RuboCop cops.
--except-cops Specify a comma-separated cops list to
skip checking for violations of the listed
RuboCop cops.
-d, --debug Display any debugging information.
-q, --quiet Make some output more quiet.
-v, --verbose Make some output more verbose.
-h, --help Show this message.
I like this. Thanks, @EricFromCanada! This will also help to solve the long line issue I mentioned above. I'll likely use this style for the generated completions. |
@MikeMcQuaid definitely agree. This has come up a few more times recently. Should I prioritize this over finishing completions? |
@Rylan12 Up to you! I think maybe but I don't feel strongly either way? |
Okay, I will. No reason not to, I think. |
Be consistent and indicate in usage banner when a command accepts multiple named args.
It was already done for a bunch of commands, this PR extends the idea to other commands.
Also fix some commands not accepting multiple named args even though they should and indeed were before.
I think some bugs was introduced in #10288, cc @Rylan12
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?