cmd,dev-cmd: indicate multiple named args in usage banner by dawidd6 · Pull Request #10337 · Homebrew/brew

dawidd6 · 2021-01-15T12:09:22Z

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

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?
Have you successfully run brew man locally and committed any changes?

BrewTestBot · 2021-01-15T12:09:41Z

Review period will end on 2021-01-18 at 12:09:22 UTC.

BrewTestBot · 2021-01-15T12:10:26Z

Review period skipped due to critical label.

dawidd6 · 2021-01-15T12:10:36Z

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

EricFromCanada · 2021-01-15T14:18:28Z

Instead of having repeated items in the usage banner, use the following convention:

<formula> for one required formula
<formula> [...] for at least one formula
[<formula> ...] for one or more optional formulae

MikeMcQuaid

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).

MikeMcQuaid · 2021-01-15T14:22:51Z

<formula> for one required formula

<formula> [...] for at least one formula

[<formula> ...] for one or more optional formulae

This is nice.

dawidd6 · 2021-01-15T14:56:43Z

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 pr-" commands. Since there is an approval I'm merging this PR as is for now.

Rylan12

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!

Rylan12 · 2021-01-15T15:20:56Z

Library/Homebrew/cmd/desc.rb

      conflicts "--search", "--name", "--description"

-      named_args :formula
+      named_args :formula, min: 1


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)

Rylan12 · 2021-01-15T15:23:11Z

Library/Homebrew/cmd/desc.rb

    Homebrew::CLI::Parser.new do
      usage_banner <<~EOS
-        `desc` [<options>] (<text>|`/`<text>`/`|<formula>)
+        `desc` [<options>] <text>|`/`<text>`/`|<formula> [<text>|`/`<text>`/`|<formula> ...]


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)

Rylan12 · 2021-01-15T15:24:22Z

Library/Homebrew/cmd/switch.rb

      EOS

-      named 2
+      named_args number: 2


Fair enough. I didn't bother because it's disabled and will be removed, but no harm. Same for brew diy

Rylan12 · 2021-01-15T15:27:32Z

Library/Homebrew/dev-cmd/style.rb

    Homebrew::CLI::Parser.new do
      usage_banner <<~EOS
-        `style` [<options>] [<file>|<tap>|<formula>]
+        `style` [<options>] [<file>|<tap>|<formula>|<cask>] [<file>|<tap>|<formula>|<cask> ...]


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.

Rylan12 · 2021-01-15T15:31:20Z

Instead of having repeated items in the usage banner, use the following convention:

<formula> for one required formula

<formula> [...] for at least one formula

[<formula> ...] for one or more optional formulae

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.

Rylan12 · 2021-01-15T15:32:09Z

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).

@MikeMcQuaid definitely agree. This has come up a few more times recently. Should I prioritize this over finishing completions?

MikeMcQuaid · 2021-01-15T16:00:30Z

@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?

Rylan12 · 2021-01-15T16:07:09Z

Okay, I will. No reason not to, I think.

dawidd6 added 3 commits January 15, 2021 13:04

cmd: indicate multiple named args in usage banner

3323724

dev-cmd: indicate multiple named args in usage banner

cf654da

cmd,dev-cmd: regenerate manpages and docs

f949d57

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 15, 2021

dawidd6 added the critical Critical change which should be shipped as soon as possible. label Jan 15, 2021

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 15, 2021

BrewTestBot approved these changes Jan 15, 2021

View reviewed changes

deb-cmd/irb: remove named_args :none

ce2dd73

seems like this command takes named args somewhere

MikeMcQuaid approved these changes Jan 15, 2021

View reviewed changes

dawidd6 merged commit f7f72b1 into Homebrew:master Jan 15, 2021

dawidd6 deleted the cmd_dev-cmd_usages branch January 15, 2021 14:56

Rylan12 reviewed Jan 15, 2021

View reviewed changes

BrewTestBot added the outdated PR was locked due to age label Feb 15, 2021

Homebrew locked as resolved and limited conversation to collaborators Feb 15, 2021

Uh oh!

Conversation

dawidd6 commented Jan 15, 2021

Uh oh!

BrewTestBot commented Jan 15, 2021

Uh oh!

BrewTestBot commented Jan 15, 2021

Uh oh!

dawidd6 commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricFromCanada commented Jan 15, 2021

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Jan 15, 2021

Uh oh!

dawidd6 commented Jan 15, 2021

Uh oh!

Rylan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Rylan12 Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Rylan12 Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Rylan12 Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Rylan12 Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Rylan12 commented Jan 15, 2021

Uh oh!

Rylan12 commented Jan 15, 2021

Uh oh!

MikeMcQuaid commented Jan 15, 2021

Uh oh!

Rylan12 commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dawidd6 commented Jan 15, 2021 •

edited

Loading