Refactor cask command parsing logic.

Library/Homebrew/cask/cmd.rb

@@ -17,6 +17,7 @@
 require "cask/cmd/doctor"
 require "cask/cmd/edit"
 require "cask/cmd/fetch"
+require "cask/cmd/help"
 require "cask/cmd/home"
 require "cask/cmd/info"
 require "cask/cmd/install"
@@ -37,9 +38,7 @@ class Cmd
     ALIASES = {
       "ls"       => "list",
       "homepage" => "home",
-      "-S"       => "search",    # verb starting with "-" is questionable
-      "up"       => "update",
-      "instal"   => "install",   # gem does the same
+      "instal"   => "install", # gem does the same
       "uninstal" => "uninstall",
       "rm"       => "uninstall",
       "remove"   => "uninstall",
@@ -86,77 +85,70 @@ def self.commands
     def self.lookup_command(command_name)
       @lookup ||= Hash[commands.zip(command_classes)]
       command_name = ALIASES.fetch(command_name, command_name)
-      @lookup.fetch(command_name, command_name)
+      @lookup.fetch(command_name, nil)
     end
 
-    def self.run_command(command, *args)
-      return command.run(*args) if command.respond_to?(:run)
+    def self.run(*args)
+      new(*args).run
+    end
 
-      tap_cmd_directories = Tap.cmd_directories
+    def initialize(*args)
+      @args = process_options(*args)
+    end
 
-      path = PATH.new(tap_cmd_directories, ENV["HOMEBREW_PATH"])
+    def find_external_command(command)
+      @tap_cmd_directories ||= Tap.cmd_directories
+      @path ||= PATH.new(@tap_cmd_directories, ENV["HOMEBREW_PATH"])
 
-      external_ruby_cmd = tap_cmd_directories.map { |d| d/"brewcask-#{command}.rb" }
-                                             .find(&:file?)
-      external_ruby_cmd ||= which("brewcask-#{command}.rb", path)
+      external_ruby_cmd = @tap_cmd_directories.map { |d| d/"brewcask-#{command}.rb" }
+                                              .find(&:file?)
+      external_ruby_cmd ||= which("brewcask-#{command}.rb", @path)
 
       if external_ruby_cmd
-        require external_ruby_cmd
-
-        klass = begin
-          const_get(command.to_s.capitalize.to_sym)
-        rescue NameError
-          # External command is a stand-alone Ruby script.
-          return
-        end
-
-        return klass.run(*args)
-      end
-
-      if external_command = which("brewcask-#{command}", path)
-        exec external_command, *ARGV[1..-1]
+        ExternalRubyCommand.new(command, external_ruby_cmd)
+      elsif external_command = which("brewcask-#{command}", @path)
+        ExternalCommand.new(external_command)
       end
-
-      NullCommand.new(command, *args).run
     end
 
-    def self.run(*args)
-      new(*args).run
-    end
+    def detect_internal_command(*args)
+      args.each_with_index do |arg, i|
+        if command = self.class.lookup_command(arg)
+          args.delete_at(i)
+          return [command, args]
+        elsif !arg.start_with?("-")
+          break
+        end
+      end
 
-    def initialize(*args)
-      @args = process_options(*args)
+      nil
     end
 
-    def detect_command_and_arguments(*args)
-      command = args.find do |arg|
-        if self.class.commands.include?(arg)
-          true
-        else
-          break unless arg.start_with?("-")
+    def detect_external_command(*args)
+      args.each_with_index do |arg, i|
+        if command = find_external_command(arg)
+          args.delete_at(i)
+          return [command, args]
+        elsif !arg.start_with?("-")
+          break
         end
       end
 
-      if index = args.index(command)
-        args.delete_at(index)
-      end
-
-      [*command, *args]
+      nil
     end
 
     def run
-      command_name, *args = detect_command_and_arguments(*@args)
-      command = if help?
-        args.unshift(command_name) unless command_name.nil?
-        "help"
-      else
-        self.class.lookup_command(command_name)
-      end
-
       MacOS.full_version = ENV["MACOS_VERSION"] unless ENV["MACOS_VERSION"].nil?
-
       Tap.default_cask_tap.install unless Tap.default_cask_tap.installed?
-      self.class.run_command(command, *args)
+
+      args = @args.dup
+      command, args = detect_internal_command(*args) || detect_external_command(*args) || [NullCommand.new, args]
+
+      if help?
+        puts command.help
+      else
+        command.run(*args)
+      end
     rescue CaskError, MethodDeprecatedError, ArgumentError, OptionParser::InvalidOption => e
       onoe e.message
       $stderr.puts e.backtrace if ARGV.debug?
@@ -190,16 +182,18 @@ def self.nice_listing(cask_list)
     def process_options(*args)
       exclude_regex = /^\-\-#{Regexp.union(*Config::DEFAULT_DIRS.keys.map(&Regexp.public_method(:escape)))}=/
 
-      all_args = Shellwords.shellsplit(ENV.fetch("HOMEBREW_CASK_OPTS", ""))
-                           .reject { |arg| arg.match?(exclude_regex) } + args
-
       non_options = []
 
-      if idx = all_args.index("--")
-        non_options += all_args.drop(idx)
-        all_args = all_args.first(idx)
+      if idx = args.index("--")
+        non_options += args.drop(idx)
+        args = args.first(idx)
       end
 
+      cask_opts = Shellwords.shellsplit(ENV.fetch("HOMEBREW_CASK_OPTS", ""))
+                            .reject { |arg| arg.match?(exclude_regex) }
+
+      all_args = cask_opts + args
+
       remaining = all_args.select do |arg|
         !process_arguments([arg]).empty?
       rescue OptionParser::InvalidOption, OptionParser::MissingArgument, OptionParser::AmbiguousOption
@@ -209,53 +203,45 @@ def process_options(*args)
       remaining + non_options
     end
 
-    class NullCommand
-      def initialize(command, *args)
-        @command = command
-        @args = args
+    class ExternalRubyCommand
+      def initialize(command, path)
+        @command_name = command.to_s.capitalize.to_sym
+        @path = path
       end
 
-      def run(*)
-        purpose
-        usage
+      def run(*args)
+        require @path
 
-        return if @command.nil?
-
-        if @command == "help"
-          return if @args.empty?
-
-          raise ArgumentError, "help does not take arguments." if @args.length
+        klass = begin
+          Cmd.const_get(@command_name)
+        rescue NameError
+          return
         end
 
-        raise ArgumentError, "Unknown Cask command: #{@command}"
+        klass.run(*args)
       end
+    end
 
-      def purpose
-        puts <<~EOS
-          Homebrew Cask provides a friendly CLI workflow for the administration
-          of macOS applications distributed as binaries.
-
-        EOS
+    class ExternalCommand
+      def initialize(path)
+        @path = path
       end
 
-      def usage
-        max_command_len = Cmd.commands.map(&:length).max
-
-        puts "Commands:\n\n"
-        Cmd.command_classes.each do |klass|
-          next unless klass.visible
-
-          puts "    #{klass.command_name.ljust(max_command_len)}  #{_help_for(klass)}"
-        end
-        puts %Q(\nSee also "man brew-cask")
+      def run(*)
+        exec @path, *ARGV[1..-1]
       end
+    end
 
-      def help
-        ""
-      end
+    class NullCommand
+      def run(*args)
+        if args.empty?
+          ofail "No subcommand given.\n"
+        else
+          ofail "Unknown subcommand: #{args.first}"
+        end
 
-      def _help_for(klass)
-        klass.respond_to?(:help) ? klass.help : nil
+        $stderr.puts
+        $stderr.puts Help.usage
       end
     end
   end

Library/Homebrew/cask/cmd/abstract_command.rb

@@ -24,7 +24,7 @@ def self.abstract?
         name.split("::").last.match?(/^Abstract[^a-z]/)
       end
 
-      def self.visible
+      def self.visible?
         true
       end
 

Library/Homebrew/cask/cmd/abstract_internal_command.rb

@@ -7,7 +7,7 @@ def self.command_name
         super.sub(/^internal_/i, "_")
       end
 
-      def self.visible
+      def self.visible?
         false
       end
     end

Library/Homebrew/cask/cmd/help.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+module Cask
+  class Cmd
+    class Help < AbstractCommand
+      def initialize(*)
+        super
+        return if args.empty?
+
+        raise ArgumentError, "#{self.class.command_name} does not take arguments."
+      end
+
+      def run
+        puts self.class.purpose
+        puts
+        puts self.class.usage
+      end
+
+      def self.purpose
+        <<~EOS
+          Homebrew Cask provides a friendly CLI workflow for the administration
+          of macOS applications distributed as binaries.
+        EOS
+      end
+
+      def self.usage
+        max_command_len = Cmd.commands.map(&:length).max
+
+        "Commands:\n" +
+          Cmd.command_classes
+             .select(&:visible?)
+             .map { |klass| "    #{klass.command_name.ljust(max_command_len)}  #{klass.help}\n" }
+             .join +
+          %Q(\nSee also "man brew-cask")
+      end
+
+      def self.help
+        "print help strings for commands"
+      end
+    end
+  end
+end

Library/Homebrew/cask/cmd/internal_help.rb

@@ -14,17 +14,13 @@ def run
         max_command_len = Cmd.commands.map(&:length).max
         puts "Unstable Internal-use Commands:\n\n"
         Cmd.command_classes.each do |klass|
-          next if klass.visible
+          next if klass.visible?
 
-          puts "    #{klass.command_name.ljust(max_command_len)}  #{self.class.help_for(klass)}"
+          puts "    #{klass.command_name.ljust(max_command_len)}  #{klass.help}"
         end
         puts "\n"
       end
 
-      def self.help_for(klass)
-        klass.respond_to?(:help) ? klass.help : nil
-      end
-
       def self.help
         "print help strings for unstable internal-use commands"
       end

Library/Homebrew/test/cask/cmd_spec.rb

@@ -17,7 +17,7 @@
 
   it "ignores the `--language` option, which is handled in `OS::Mac`" do
     cli = described_class.new("--language=en")
-    expect(cli).to receive(:detect_command_and_arguments).with(no_args)
+    expect(cli).to receive(:detect_internal_command).with(no_args)
     cli.run
   end
 
@@ -36,35 +36,35 @@
   end
 
   context "::run" do
-    let(:noop_command) { double("Cmd::Noop") }
-
-    before do
-      allow(described_class).to receive(:lookup_command).with("noop").and_return(noop_command)
-      allow(noop_command).to receive(:run)
-    end
-
-    it "passes `--version` along to the subcommand" do
-      version_command = double("Cmd::Version")
-      allow(described_class).to receive(:lookup_command).with("--version").and_return(version_command)
-      expect(described_class).to receive(:run_command).with(version_command)
-      described_class.run("--version")
-    end
+    let(:noop_command) { double("Cmd::Noop", run: nil) }
 
     it "prints help output when subcommand receives `--help` flag" do
-      command = described_class.new("noop", "--help")
-      expect(described_class).to receive(:run_command).with("help", "noop")
-      command.run
+      command = described_class.new("info", "--help")
+
+      expect { command.run }.to output(/displays information about the given Cask/).to_stdout
       expect(command.help?).to eq(true)
     end
 
     it "respects the env variable when choosing what appdir to create" do
+      allow(described_class).to receive(:lookup_command).with("noop").and_return(noop_command)
+
       ENV["HOMEBREW_CASK_OPTS"] = "--appdir=/custom/appdir"
 
       described_class.run("noop")
 
       expect(Cask::Config.global.appdir).to eq(Pathname.new("/custom/appdir"))
     end
 
+    it "overrides the env variable when passing --appdir directly" do
+      allow(described_class).to receive(:lookup_command).with("noop").and_return(noop_command)
+
+      ENV["HOMEBREW_CASK_OPTS"] = "--appdir=/custom/appdir"
+
+      described_class.run("noop", "--appdir=/even/more/custom/appdir")
+
+      expect(Cask::Config.global.appdir).to eq(Pathname.new("/even/more/custom/appdir"))
+    end
+
     it "exits with a status of 1 when something goes wrong" do
       allow(described_class).to receive(:lookup_command).and_raise(Cask::CaskError)
       command = described_class.new("noop")
@@ -73,8 +73,8 @@
     end
   end
 
-  it "provides a help message for all visible commands" do
-    described_class.command_classes.select(&:visible).each do |command_class|
+  it "provides a help message for all commands" do
+    described_class.command_classes.each do |command_class|
       expect(command_class.help).to match(/\w+/), command_class.name
     end
   end