From 49b9b6cf3fa5d01db46b0645696aca072dfa8665 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sat, 23 Jan 2021 16:59:33 +0000 Subject: [PATCH 1/5] dev-cmd/extract: Improve the usage instructions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 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. --- Library/Homebrew/dev-cmd/extract.rb | 3 ++- completions/bash/brew | 1 - docs/Manpage.md | 2 +- manpages/brew.1 | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/dev-cmd/extract.rb b/Library/Homebrew/dev-cmd/extract.rb index 59a9519b67..d149fd432a 100644 --- a/Library/Homebrew/dev-cmd/extract.rb +++ b/Library/Homebrew/dev-cmd/extract.rb @@ -83,6 +83,7 @@ module Homebrew sig { returns(CLI::Parser) } def extract_args Homebrew::CLI::Parser.new do + usage_banner "`extract` [<--version>`=`] [<--force>] " description <<~EOS Look through repository history to find the most recent version of and create a copy in `/Formula/``@``.rb`. If the tap is not @@ -95,7 +96,7 @@ module Homebrew switch "-f", "--force", description: "Overwrite the destination formula if it already exists." - named_args :formula, number: 2 + named_args number: 2 end end diff --git a/completions/bash/brew b/completions/bash/brew index 7941e54fee..a086378a02 100644 --- a/completions/bash/brew +++ b/completions/bash/brew @@ -898,7 +898,6 @@ _brew_extract() { return ;; esac - __brew_complete_formulae } _brew_fetch() { diff --git a/docs/Manpage.md b/docs/Manpage.md index bd1bb814fe..f04a6f661c 100644 --- a/docs/Manpage.md +++ b/docs/Manpage.md @@ -1019,7 +1019,7 @@ or open the Homebrew repository for editing if no formula is provided. * `--cask`: Treat all named arguments as casks. -### `extract` [*`--version`*`=`] [*`--force`*] *`formula`* ... +### `extract` [*`--version`*`=`] [*`--force`*] *`formula`* *`tap`* Look through repository history to find the most recent version of *`formula`* and create a copy in *`tap`*`/Formula/`*`formula`*`@`*`version`*`.rb`. If the tap is not diff --git a/manpages/brew.1 b/manpages/brew.1 index e6c0afc127..986c9ce31e 100644 --- a/manpages/brew.1 +++ b/manpages/brew.1 @@ -1417,7 +1417,7 @@ Treat all named arguments as formulae\. \fB\-\-cask\fR Treat all named arguments as casks\. . -.SS "\fBextract\fR [\fI\-\-version\fR\fB=\fR] [\fI\-\-force\fR] \fIformula\fR \.\.\." +.SS "\fBextract\fR [\fI\-\-version\fR\fB=\fR] [\fI\-\-force\fR] \fIformula\fR \fItap\fR" Look through repository history to find the most recent version of \fIformula\fR and create a copy in \fItap\fR\fB/Formula/\fR\fIformula\fR\fB@\fR\fIversion\fR\fB\.rb\fR\. If the tap is not installed yet, attempt to install/clone the tap before continuing\. To extract a formula from a tap that is not \fBhomebrew/core\fR use its fully\-qualified form of \fIuser\fR\fB/\fR\fIrepo\fR\fB/\fR\fIformula\fR\. . .TP From b7b624c9bfb5c6a28776ffed8f1811fd82e5b022 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 23 Jan 2021 15:06:44 -0500 Subject: [PATCH 2/5] parser: clarify errors for invalid number of args --- Library/Homebrew/cli/parser.rb | 43 +++++++++++--------- Library/Homebrew/test/cli/parser_spec.rb | 50 ++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 23 deletions(-) diff --git a/Library/Homebrew/cli/parser.rb b/Library/Homebrew/cli/parser.rb index c687ea688b..9b4f41e6e4 100644 --- a/Library/Homebrew/cli/parser.rb +++ b/Library/Homebrew/cli/parser.rb @@ -599,21 +599,16 @@ module Homebrew end def check_named_args(args) + types = Array(@named_args_type).map do |type| + next type if type.is_a? Symbol + + :subcommand + end.compact.uniq + exception = if @min_named_args && args.size < @min_named_args - if @named_args_type.present? - types = Array(@named_args_type) - if types.any? { |arg| arg.is_a? String } - MinNamedArgumentsError.new(@min_named_args) - else - list = types.map { |type| type.to_s.tr("_", " ") } - list = list.to_sentence two_words_connector: " or ", last_word_connector: " or " - UsageError.new("this command requires a #{list} argument") - end - else - MinNamedArgumentsError.new(@min_named_args) - end + MinNamedArgumentsError.new(@min_named_args, types: types, exact: @min_named_args == @max_named_args) elsif @max_named_args && args.size > @max_named_args - MaxNamedArgumentsError.new(@max_named_args) + MaxNamedArgumentsError.new(@max_named_args, types: types) end raise exception if exception @@ -688,13 +683,17 @@ module Homebrew class MaxNamedArgumentsError < UsageError extend T::Sig - sig { params(maximum: Integer).void } - def initialize(maximum) + sig { params(maximum: Integer, types: T::Array[Symbol]).void } + def initialize(maximum, types: []) super case maximum when 0 "This command does not take named arguments." else - "This command does not take more than #{maximum} named #{"argument".pluralize(maximum)}" + types << :named if types.empty? + arg_types = types.map { |type| type.to_s.tr("_", " ") } + .to_sentence two_words_connector: " or ", last_word_connector: " or " + + "This command does not take more than #{maximum} #{arg_types} #{"argument".pluralize(maximum)}." end end end @@ -702,9 +701,15 @@ module Homebrew class MinNamedArgumentsError < UsageError extend T::Sig - sig { params(minimum: Integer).void } - def initialize(minimum) - super "This command requires at least #{minimum} named #{"argument".pluralize(minimum)}." + sig { params(minimum: Integer, types: T::Array[Symbol], exact: T::Boolean).void } + def initialize(minimum, types: [], exact: false) + number_phrase = exact ? "exactly" : "at least" + + types << :named if types.empty? + arg_types = types.map { |type| type.to_s.tr("_", " ") } + .to_sentence two_words_connector: " or ", last_word_connector: " or " + + super "This command requires #{number_phrase} #{minimum} #{arg_types} #{"argument".pluralize(minimum)}." end end end diff --git a/Library/Homebrew/test/cli/parser_spec.rb b/Library/Homebrew/test/cli/parser_spec.rb index 4b1ace9663..45f31c5223 100644 --- a/Library/Homebrew/test/cli/parser_spec.rb +++ b/Library/Homebrew/test/cli/parser_spec.rb @@ -475,18 +475,58 @@ describe Homebrew::CLI::Parser do expect { parser_none.parse([]) }.not_to raise_error end + it "displays the correct error message with no arg types and min" do + parser = described_class.new do + named_args min: 2 + end + expect { parser.parse([]) }.to raise_error( + Homebrew::CLI::MinNamedArgumentsError, /This command requires at least 2 named arguments/ + ) + end + + it "displays the correct error message with no arg types and number" do + parser = described_class.new do + named_args number: 2 + end + expect { parser.parse([]) }.to raise_error( + Homebrew::CLI::MinNamedArgumentsError, /This command requires exactly 2 named arguments/ + ) + end + + it "displays the correct error message with no arg types and max" do + parser = described_class.new do + named_args max: 1 + end + expect { parser.parse(%w[foo bar]) }.to raise_error( + Homebrew::CLI::MaxNamedArgumentsError, /This command does not take more than 1 named argument/ + ) + end + it "displays the correct error message with an array of strings" do parser = described_class.new do - named_args %w[on off], min: 1 + named_args %w[on off], number: 1 end - expect { parser.parse([]) }.to raise_error(Homebrew::CLI::MinNamedArgumentsError) + expect { parser.parse([]) }.to raise_error( + Homebrew::CLI::MinNamedArgumentsError, /This command requires exactly 1 subcommand/ + ) end it "displays the correct error message with an array of symbols" do parser = described_class.new do named_args [:formula, :cask], min: 1 end - expect { parser.parse([]) }.to raise_error(UsageError, /this command requires a formula or cask argument/) + expect { parser.parse([]) }.to raise_error( + Homebrew::CLI::MinNamedArgumentsError, /This command requires at least 1 formula or cask argument/ + ) + end + + it "displays the correct error message with an array of symbols and max" do + parser = described_class.new do + named_args [:formula, :cask], max: 1 + end + expect { parser.parse(%w[foo bar]) }.to raise_error( + Homebrew::CLI::MaxNamedArgumentsError, /This command does not take more than 1 formula or cask argument/ + ) end end @@ -509,7 +549,9 @@ describe Homebrew::CLI::Parser do formula_parser = described_class.new do named :formula end - expect { formula_parser.parse([]) }.to raise_error(UsageError, /this command requires a formula argument/) + expect { formula_parser.parse([]) }.to raise_error( + Homebrew::CLI::MinNamedArgumentsError, /This command requires exactly 1 formula argument/ + ) end it "doesn't allow more than the specified number of arguments" do From f295e36a2206773b90d431dfaf776ffb0dba404d Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 23 Jan 2021 15:10:14 -0500 Subject: [PATCH 3/5] extract: re-add named arg types --- Library/Homebrew/dev-cmd/extract.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/dev-cmd/extract.rb b/Library/Homebrew/dev-cmd/extract.rb index d149fd432a..a2f695282d 100644 --- a/Library/Homebrew/dev-cmd/extract.rb +++ b/Library/Homebrew/dev-cmd/extract.rb @@ -96,7 +96,7 @@ module Homebrew switch "-f", "--force", description: "Overwrite the destination formula if it already exists." - named_args number: 2 + named_args [:formula, :tap], number: 2 end end From 01e894e9c6bf8e8b1b2cf714205e2b93bf0d4795 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 23 Jan 2021 15:25:56 -0500 Subject: [PATCH 4/5] parser: create NumberOfNamedArgumentsError And commit `brew man` changes --- Library/Homebrew/cli/parser.rb | 28 ++++++++++++++++++------ Library/Homebrew/test/cli/parser_spec.rb | 14 ++++++------ completions/bash/brew | 2 ++ 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/Library/Homebrew/cli/parser.rb b/Library/Homebrew/cli/parser.rb index 9b4f41e6e4..33fed1bfb8 100644 --- a/Library/Homebrew/cli/parser.rb +++ b/Library/Homebrew/cli/parser.rb @@ -605,8 +605,11 @@ module Homebrew :subcommand end.compact.uniq - exception = if @min_named_args && args.size < @min_named_args - MinNamedArgumentsError.new(@min_named_args, types: types, exact: @min_named_args == @max_named_args) + exception = if @min_named_args && @max_named_args && @min_named_args == @max_named_args && + args.size != @max_named_args + NumberOfNamedArgumentsError.new(@min_named_args, types: types) + elsif @min_named_args && args.size < @min_named_args + MinNamedArgumentsError.new(@min_named_args, types: types) elsif @max_named_args && args.size > @max_named_args MaxNamedArgumentsError.new(@max_named_args, types: types) end @@ -701,15 +704,26 @@ module Homebrew class MinNamedArgumentsError < UsageError extend T::Sig - sig { params(minimum: Integer, types: T::Array[Symbol], exact: T::Boolean).void } - def initialize(minimum, types: [], exact: false) - number_phrase = exact ? "exactly" : "at least" - + sig { params(minimum: Integer, types: T::Array[Symbol]).void } + def initialize(minimum, types: []) types << :named if types.empty? arg_types = types.map { |type| type.to_s.tr("_", " ") } .to_sentence two_words_connector: " or ", last_word_connector: " or " - super "This command requires #{number_phrase} #{minimum} #{arg_types} #{"argument".pluralize(minimum)}." + super "This command requires at least #{minimum} #{arg_types} #{"argument".pluralize(minimum)}." + end + end + + class NumberOfNamedArgumentsError < UsageError + extend T::Sig + + sig { params(minimum: Integer, types: T::Array[Symbol]).void } + def initialize(minimum, types: []) + types << :named if types.empty? + arg_types = types.map { |type| type.to_s.tr("_", " ") } + .to_sentence two_words_connector: " or ", last_word_connector: " or " + + super "This command requires exactly #{minimum} #{arg_types} #{"argument".pluralize(minimum)}." end end end diff --git a/Library/Homebrew/test/cli/parser_spec.rb b/Library/Homebrew/test/cli/parser_spec.rb index 45f31c5223..c50c9fe2f7 100644 --- a/Library/Homebrew/test/cli/parser_spec.rb +++ b/Library/Homebrew/test/cli/parser_spec.rb @@ -455,11 +455,11 @@ describe Homebrew::CLI::Parser do end it "doesn't accept fewer than the passed number of arguments" do - expect { parser_number.parse([]) }.to raise_error(Homebrew::CLI::MinNamedArgumentsError) + expect { parser_number.parse([]) }.to raise_error(Homebrew::CLI::NumberOfNamedArgumentsError) end it "doesn't accept more than the passed number of arguments" do - expect { parser_number.parse(["foo", "bar"]) }.to raise_error(Homebrew::CLI::MaxNamedArgumentsError) + expect { parser_number.parse(["foo", "bar"]) }.to raise_error(Homebrew::CLI::NumberOfNamedArgumentsError) end it "accepts the passed number of arguments" do @@ -489,7 +489,7 @@ describe Homebrew::CLI::Parser do named_args number: 2 end expect { parser.parse([]) }.to raise_error( - Homebrew::CLI::MinNamedArgumentsError, /This command requires exactly 2 named arguments/ + Homebrew::CLI::NumberOfNamedArgumentsError, /This command requires exactly 2 named arguments/ ) end @@ -507,7 +507,7 @@ describe Homebrew::CLI::Parser do named_args %w[on off], number: 1 end expect { parser.parse([]) }.to raise_error( - Homebrew::CLI::MinNamedArgumentsError, /This command requires exactly 1 subcommand/ + Homebrew::CLI::NumberOfNamedArgumentsError, /This command requires exactly 1 subcommand/ ) end @@ -542,7 +542,7 @@ describe Homebrew::CLI::Parser do end it "doesn't allow less than the specified number of arguments" do - expect { parser.parse([]) }.to raise_error(Homebrew::CLI::MinNamedArgumentsError) + expect { parser.parse([]) }.to raise_error(Homebrew::CLI::NumberOfNamedArgumentsError) end it "treats a symbol as a single argument of the specified type" do @@ -550,12 +550,12 @@ describe Homebrew::CLI::Parser do named :formula end expect { formula_parser.parse([]) }.to raise_error( - Homebrew::CLI::MinNamedArgumentsError, /This command requires exactly 1 formula argument/ + Homebrew::CLI::NumberOfNamedArgumentsError, /This command requires exactly 1 formula argument/ ) end it "doesn't allow more than the specified number of arguments" do - expect { parser.parse(["foo", "bar"]) }.to raise_error(Homebrew::CLI::MaxNamedArgumentsError) + expect { parser.parse(["foo", "bar"]) }.to raise_error(Homebrew::CLI::NumberOfNamedArgumentsError) end end diff --git a/completions/bash/brew b/completions/bash/brew index a086378a02..323a30a082 100644 --- a/completions/bash/brew +++ b/completions/bash/brew @@ -898,6 +898,8 @@ _brew_extract() { return ;; esac + __brew_complete_formulae + __brew_complete_tapped } _brew_fetch() { From 7a0471f4c90a2c0df2366d2bf12f7326b4f9ba29 Mon Sep 17 00:00:00 2001 From: Rylan Polster Date: Sat, 23 Jan 2021 17:50:41 -0500 Subject: [PATCH 5/5] cask/cmd: fix tests --- Library/Homebrew/test/cask/cmd/create_spec.rb | 2 +- Library/Homebrew/test/cask/cmd/edit_spec.rb | 2 +- .../test/cask/cmd/shared_examples/requires_cask_token.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/test/cask/cmd/create_spec.rb b/Library/Homebrew/test/cask/cmd/create_spec.rb index d8fa5cb425..054d7dca4a 100644 --- a/Library/Homebrew/test/cask/cmd/create_spec.rb +++ b/Library/Homebrew/test/cask/cmd/create_spec.rb @@ -47,7 +47,7 @@ describe Cask::Cmd::Create, :cask do it "raises an exception when more than one Cask is given" do expect { described_class.run("additional-cask", "another-cask") - }.to raise_error(UsageError, /Only one cask can be created at a time\./) + }.to raise_error(Homebrew::CLI::NumberOfNamedArgumentsError) end it "raises an exception when the Cask already exists" do diff --git a/Library/Homebrew/test/cask/cmd/edit_spec.rb b/Library/Homebrew/test/cask/cmd/edit_spec.rb index 18d390ee74..8d86bf4649 100644 --- a/Library/Homebrew/test/cask/cmd/edit_spec.rb +++ b/Library/Homebrew/test/cask/cmd/edit_spec.rb @@ -21,7 +21,7 @@ describe Cask::Cmd::Edit, :cask do it "raises an error when given more than one argument" do expect { described_class.new("local-caffeine", "local-transmission") - }.to raise_error(UsageError, /Only one cask can be edited at a time\./) + }.to raise_error(Homebrew::CLI::NumberOfNamedArgumentsError) end it "raises an exception when the Cask doesn't exist" do diff --git a/Library/Homebrew/test/cask/cmd/shared_examples/requires_cask_token.rb b/Library/Homebrew/test/cask/cmd/shared_examples/requires_cask_token.rb index c3eb1a0708..bfb750ab1e 100644 --- a/Library/Homebrew/test/cask/cmd/shared_examples/requires_cask_token.rb +++ b/Library/Homebrew/test/cask/cmd/shared_examples/requires_cask_token.rb @@ -6,7 +6,7 @@ shared_examples "a command that requires a Cask token" do it "raises an exception " do expect { described_class.run - }.to raise_error(UsageError, /this command requires a .*cask.* argument/) + }.to raise_error(UsageError, /This command requires .*cask.* argument/) end end end