cmd/list: Constrain `-lrt` options to formulae #10133

issyl0 · 2020-12-24T13:40:07Z

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?

Fixes brew list -l produces short output for casks, doesn't print totals as claimed in help #10116 (part two).
These are documented as only working on formulae, but users expect the same options (long format, reverse order or sort by modified time) to be passed to both formulae and casks in the default brew list. The output looks weird as they're not. Hence, constrain these to be --formula-only.
This was added originally in 5adb76a, but disappeared later (I've not yet dug through and found why).
This also fixes the CLI parser's handling of error messages for short options (assuming they're only a single character). Before, the error messages for this missing --formula option on brew list -l was Error: Invalid usage: --lcannot be passed without--formula.. ~~I don't much like this code, but I'll revisit it later - I have to run for a 🚌 🚏 now!~~

- These are documented as only working on formulae, but users expect the same options (long format, reverse order or sort by modified time) to be passed to both formulae and casks in the default `brew list`. The output looks weird as they're not. Hence, constrain these to be `--formula`-only. - This was added originally in 5adb76a, but then disappeared.

- This avoids error messages like: ``` ➜ brew list -l Error: Invalid usage: `--l` cannot be passed without `--formula`. ```

BrewTestBot · 2020-12-24T13:40:42Z

Review period will end on 2020-12-25 at 13:40:07 UTC.

MikeMcQuaid

👏🏻 ❤️

- This reads nicer (to me).

Rylan12 · 2020-12-24T16:06:53Z

FYI @MikeMcQuaid you added these depends_on lines in #9035 and then promptly removed them (two hours later) in #9038. Just want to make sure that we don't re-introduce any issues that then cause this to be removed once again.

Based on glancing at the issues, if brew list -l fails (because it required --formula) and brew list <formula> --versions works, I think both issues have been solved.

Edit: it looks like #9038 just removed the depends_on line from more options than it needed to. I think this is good.

issyl0 · 2020-12-24T16:12:58Z

@Rylan12 Both work with these changes:

➜ brew list -l
[...]
Error: Invalid usage: `-l` cannot be passed without `--formula`.
➜ brew list ruby --versions
ruby 2.7.2

Rylan12

Works for me too!

Rylan12 · 2020-12-24T16:19:24Z

Library/Homebrew/cli/parser.rb

-        arg1 = "--#{arg1.tr("_", "-")}"
-        arg2 = "--#{arg2.tr("_", "-")}"
+        arg1 = dashes(arg1) + arg1.tr("_", "-")
+        arg2 = dashes(arg2) + arg2.tr("_", "-")


You might be able to use the name_to_option method here

Hmm, wait. It looks like this is already being called...

brew/Library/Homebrew/cli/parser.rb

Line 455 in 27afcf5

raise OptionConflictError, violations.map(&method(:name_to_option)) unless select_cli_arg

Maybe it doesn't do what I thought...

The tr("_", "-") part is not correct for e.g. --screen_saverdir.

Huh. That must have been broken before now. Can you give me a full example command that uses that so I can try to figure it out?

@issyl0 I think this will work for the check_constraints method which will remove the need for this:

I just changed primary and secondary to name_to_option(primary) and name_to_option(secondary) in the raises)

def check_constraints @constraints.each do |primary, secondary, constraint_type| primary_passed = option_passed?(primary) secondary_passed = option_passed?(secondary) if :mandatory.equal?(constraint_type) && primary_passed && !secondary_passed raise OptionConstraintError.new(name_to_option(primary), name_to_option(secondary)) end raise OptionConstraintError.new(name_to_option(primary), name_to_option(secondary), missing: true) if secondary_passed && !primary_passed end end

This doesn't fix the issue that @reitermarkus raised, though.

I think the issue @reitermarkus raised was pre-existing as I didn't change any of the code for the arg.tr("_", "-") stuff, so we can tackle that in another PR!

It looks like these are the options that will have this issue:

--input_methoddir

--internet_plugindir

--audio_unit_plugindir

--vst_plugindir

--vst3_plugindir

--screen_saverdir

They're all cask options. We may want to check in name_to_option to see if the option matched one of those. If so, don't do the tr. Separate PR makes sense I think

Rylan12

Oops, my bad with the suggestion. Otherwise looks good!

Library/Homebrew/cli/parser.rb

- There's already a method on `CLI::Parser`, we don't need to hand-roll the "number of dashes" detection. Co-authored-by: Rylan Polster <[email protected]>

issyl0 · 2020-12-24T16:54:38Z

No problem, I should have noticed that line was getting a bit long. I fixed it in a different way.

issyl0 · 2020-12-24T17:00:05Z

Given this fixes a bug, I'm adding the critical label. 🟥

BrewTestBot · 2020-12-24T17:00:34Z

Review period ended.

MikeMcQuaid · 2020-12-25T13:54:03Z

FYI @MikeMcQuaid you added these depends_on lines in #9035 and then promptly removed them (two hours later) in #9038. Just want to make sure that we don't re-introduce any issues that then cause this to be removed once again.

@Rylan12 that was before disabling so should be fine 👍🏻

Thanks @issyl0!

issyl0 added 2 commits December 24, 2020 13:33

cli/parser: Only prefix short options with one dash

570a660

- This avoids error messages like: ``` ➜ brew list -l Error: Invalid usage: `--l` cannot be passed without `--formula`. ```

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 24, 2020

MikeMcQuaid approved these changes Dec 24, 2020

View reviewed changes

cli/parser: Improve single or multi "-" detection

27afcf5

- This reads nicer (to me).

Rylan12 approved these changes Dec 24, 2020

View reviewed changes

Library/Homebrew/cli/parser.rb Outdated Show resolved Hide resolved

cli/parser: Option-ify arg names when raising OptionConstraintError

531cae4

- There's already a method on `CLI::Parser`, we don't need to hand-roll the "number of dashes" detection. Co-authored-by: Rylan Polster <[email protected]>

issyl0 force-pushed the constrain-brew-list-lrt-options-to-formulae-only branch from a20e600 to 531cae4 Compare December 24, 2020 16:53

issyl0 added the critical Critical change which should be shipped as soon as possible. label Dec 24, 2020

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 24, 2020

BrewTestBot approved these changes Dec 24, 2020

View reviewed changes

Rylan12 approved these changes Dec 24, 2020

View reviewed changes

issyl0 enabled auto-merge December 24, 2020 17:10

issyl0 merged commit 2424e58 into Homebrew:master Dec 24, 2020

issyl0 deleted the constrain-brew-list-lrt-options-to-formulae-only branch December 24, 2020 18:14

Rylan12 mentioned this pull request Dec 24, 2020

cli/parser: improve name_to_option for some cask args #10139

Closed

8 tasks

BrewTestBot added the outdated PR was locked due to age label Jan 26, 2021

Homebrew locked as resolved and limited conversation to collaborators Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/list: Constrain `-lrt` options to formulae #10133

cmd/list: Constrain `-lrt` options to formulae #10133

issyl0 commented Dec 24, 2020 •

edited

Loading

BrewTestBot commented Dec 24, 2020

MikeMcQuaid left a comment

Rylan12 commented Dec 24, 2020 •

edited

Loading

issyl0 commented Dec 24, 2020

Rylan12 left a comment

Rylan12 Dec 24, 2020

Rylan12 Dec 24, 2020

reitermarkus Dec 24, 2020

issyl0 Dec 24, 2020

Rylan12 Dec 24, 2020 •

edited

Loading

issyl0 Dec 24, 2020

Rylan12 Dec 24, 2020

Rylan12 left a comment

issyl0 commented Dec 24, 2020

issyl0 commented Dec 24, 2020

BrewTestBot commented Dec 24, 2020 •

edited

Loading

MikeMcQuaid commented Dec 25, 2020

cmd/list: Constrain -lrt options to formulae #10133

cmd/list: Constrain -lrt options to formulae #10133

Conversation

issyl0 commented Dec 24, 2020 • edited Loading

BrewTestBot commented Dec 24, 2020

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Rylan12 commented Dec 24, 2020 • edited Loading

issyl0 commented Dec 24, 2020

Rylan12 left a comment

Choose a reason for hiding this comment

Rylan12 Dec 24, 2020

Choose a reason for hiding this comment

Rylan12 Dec 24, 2020

Choose a reason for hiding this comment

reitermarkus Dec 24, 2020

Choose a reason for hiding this comment

issyl0 Dec 24, 2020

Choose a reason for hiding this comment

Rylan12 Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

issyl0 Dec 24, 2020

Choose a reason for hiding this comment

Rylan12 Dec 24, 2020

Choose a reason for hiding this comment

Rylan12 left a comment

Choose a reason for hiding this comment

issyl0 commented Dec 24, 2020

issyl0 commented Dec 24, 2020

BrewTestBot commented Dec 24, 2020 • edited Loading

MikeMcQuaid commented Dec 25, 2020

cmd/list: Constrain `-lrt` options to formulae #10133

cmd/list: Constrain `-lrt` options to formulae #10133

issyl0 commented Dec 24, 2020 •

edited

Loading

Rylan12 commented Dec 24, 2020 •

edited

Loading

Rylan12 Dec 24, 2020 •

edited

Loading

BrewTestBot commented Dec 24, 2020 •

edited

Loading