dev-cmd/extract: Improve the usage instructions

[issyl0]

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

---

• A friend got an error message when trying to use brew extract and it wasn't immediately obvious to me why. The usage banner only mentioned the "formula" argument, which they'd provided. This improves the error message when there aren't enough arguments so that others have a chance of figuring out how to use this command without having to look at the code for extract_args.

Before:

➜ brew extract --version='1.4' libftdi
Usage: brew extract [--version=] [--force] formula ...
[...]
Error: Invalid usage: this command requires a formula argument

After:

➜ brew extract --version='1.4' libftdi
Usage: brew extract [options] formula destination_tap
[...]
Error: Invalid usage: This command requires at least 2 named arguments.

• I don't like the "at least 2" phrasing here but that's a dive into the arg parsing code that I don't have time for right now. An alternative was named_args [:formula, :destination_tap], but that gave the error message "requires formula or destination_tap" which wasn't great either. I also tried min: 2, max: 2 and that was the same "at least 2" message.

---

I still cannot run brew man locally because something in my gems is messed up and ronn doesn't exist, so I'm going to try to re-learn enough manpage syntax to edit them by hand.

[BrewTestBot]

Review period will end on 2021-01-26 at 00:00:00 UTC.

[Rylan12]

Okay, I modified the Homebrew::CLI::Parser code to clarify the wording for these error messages.

Now, MinNamedArgumentsError and MaxNamedArgumentsError accept an array of types to add additional clarity to the error message (i.e. say formula argument instead of named argument when possible). This was done before but not within the existing exception classes. This cleans it up a little bit.

I also added a new NumberOfNamedArgumentsError (I'm open to alternative names). The error message will say requires exactly N named arguments instead of using the requires at least N named arguments from MinNamedArgumentsError.

---

Here is the updated message for brew extract --version='1.4' libftdi:

$ brew extract --version='1.4' libftdi
Usage: brew extract [--version=] [--force] formula tap
[...]
Error: Invalid usage: This command requires exactly 2 formula or tap arguments.

I think this is a good middle ground. If the error message could still use improvement, I'd say the best way to do it is to manually catch the NumberOfNamedArgumentsError in the command method:

begin
  args = extract_args.parse
rescue Homebrew::CLI::NumberOfNamedArgumentsError
  raise UsageError, "new message"
end

[issyl0]

Those changes to the wording and the tests look good to me. Thanks, @Rylan12!

[Rylan12]

Thanks, @issyl0!

✅ from me on the changes here (I won't officially approve for now to avoid approving my own changes 😅)

[jonchang]

NumberOfNamedArgumentsError

NamedArgumentCountError?

Either way, 👍 on the pull request.

[Rylan12]

NamedArgumentCountError?

My only hesitation with this name is that the error is only thrown when there is an exact number of named args. A different exception is thrown when the number of arguments is above the minimum and below the maximum. To be fair, though, this is still better than what I had before (which has the same problem).

In total, there are three error types:

NumberOfNamedArgumentsError for when the number of args doesn't match the required number. This shows a message saying This command requires exactly 2 named arguments.

MinNamedArgumentsError for when the number of args hasn't yet reached the minimum number required. This shows a message saying This command requires at least 2 named arguments.

MaxNamedArgumentsError for when the number of args is greater than the maximum number allowed. This is also used when args are passed to a command that accepts no named args. This shows a message saying either This command does not take more than 2 named arguments. or This command does not take named arguments.

Maybe it makes the most sense to just have one error class called something like NamedArgumentCountError (like your suggestion) and then when thrown, intelligently gives an appropriate message based on the arg type and count parameters. This would not be difficult to do and might have some other benefits (e.g. rescuing a single error rather than multiple error types). This is already done by MaxNamedArgumentsError to modify the error message based on the number of named args allowed.

Thoughts? I'm also not sure whether MinNamedArgumentsError and MaxNamedArgumentsError are part of the public API and need to go through the deprecate/disable process or can just be replaced.

[BrewTestBot]

Review period skipped due to critical label.

[MikeMcQuaid]

Thanks all!