Merge pull request #7509 from MikeMcQuaid/cli_refactoring

Refactor CLI to remove `unless args_parsed`

Library/Homebrew/cli/args.rb

@@ -5,56 +5,44 @@ require "ostruct"
 module Homebrew
   module CLI
     class Args < OpenStruct
-      attr_reader :processed_options, :args_parsed
       # undefine tap to allow --tap argument
       undef tap
 
-      def initialize
+      def initialize(argv = ARGV.dup.freeze, set_default_args: false)
-        super
+        super()
 
-        self[:remaining] = []
-        self[:argv] = ARGV.dup.freeze
-
-        @args_parsed = false
         @processed_options = []
+
+        # Set values needed before Parser#parse has been run.
+        return unless set_default_args
+
+        self[:build_from_source?] = argv.include?("--build-from-source") || argv.include?("-s")
+        self[:build_bottle?] = argv.include?("--build-bottle")
+        self[:force_bottle?] = argv.include?("--force-bottle")
+        self[:HEAD?] = argv.include?("--HEAD")
+        self[:devel?] = argv.include?("--devel")
+        self[:universal?] = argv.include?("--universal")
+        self[:named_args] = argv.reject { |arg| arg.start_with?("-") }
+      end
+
+      def freeze_named_args!(named_args)
+        self[:named_args] = named_args
+        self[:named_args].freeze
       end
 
       def freeze_processed_options!(processed_options)
         @processed_options += processed_options
         @processed_options.freeze
-        @args_parsed = true
-      end
-
-      def option_to_name(option)
-        option.sub(/\A--?/, "")
-              .tr("-", "_")
-      end
-
-      def cli_args
-        return @cli_args if @cli_args
-
-        @cli_args = []
-        processed_options.each do |short, long|
-          option = long || short
-          switch = "#{option_to_name(option)}?".to_sym
-          flag = option_to_name(option).to_sym
-          if @table[switch] == true || @table[flag] == true
-            @cli_args << option
-          elsif @table[flag].instance_of? String
-            @cli_args << option + "=" + @table[flag]
-          elsif @table[flag].instance_of? Array
-            @cli_args << option + "=" + @table[flag].join(",")
-          end
-        end
-        @cli_args
       end
 
       def options_only
         @options_only ||= cli_args.select { |arg| arg.start_with?("-") }
+                                  .freeze
       end
 
       def flags_only
         @flags_only ||= cli_args.select { |arg| arg.start_with?("--") }
+                                .freeze
       end
 
       def passthrough
@@ -62,7 +50,7 @@ module Homebrew
       end
 
       def named
-        remaining
+        named_args || []
       end
 
       def no_named?
@@ -74,16 +62,17 @@ module Homebrew
       def collect_build_args
         build_flags = []
 
-        build_flags << "--HEAD" if head
+        build_flags << "--HEAD" if HEAD?
-        build_flags << "--universal" if build_universal
+        build_flags << "--universal" if build_universal?
-        build_flags << "--build-bottle" if build_bottle
+        build_flags << "--build-bottle" if build_bottle?
-        build_flags << "--build-from-source" if build_from_source
+        build_flags << "--build-from-source" if build_from_source?
 
         build_flags
       end
 
       def formulae
         require "formula"
+
         @formulae ||= (downcased_unique_named - casks).map do |name|
           if name.include?("/") || File.exist?(name)
             Formulary.factory(name, spec)
@@ -91,29 +80,35 @@ module Homebrew
             Formulary.find_with_priority(name, spec)
           end
         end.uniq(&:name)
+                                                      .freeze
       end
 
       def resolved_formulae
         require "formula"
+
         @resolved_formulae ||= (downcased_unique_named - casks).map do |name|
           Formulary.resolve(name, spec: spec(nil))
         end.uniq(&:name)
+                                                               .freeze
       end
 
       def formulae_paths
         @formulae_paths ||= (downcased_unique_named - casks).map do |name|
           Formulary.path(name)
         end.uniq
+                                                            .freeze
       end
 
       def casks
-        @casks ||= downcased_unique_named.grep HOMEBREW_CASK_TAP_CASK_REGEX
+        @casks ||= downcased_unique_named.grep(HOMEBREW_CASK_TAP_CASK_REGEX)
+                                         .freeze
       end
 
       def kegs
         require "keg"
         require "formula"
         require "missing_formula"
+
         @kegs ||= downcased_unique_named.map do |name|
           raise UsageError if name.empty?
 
@@ -158,7 +153,7 @@ module Homebrew
               Please delete (with rm -rf!) all but one and then try again.
             EOS
           end
-        end
+        end.freeze
       end
 
       def build_stable?
@@ -168,39 +163,40 @@ module Homebrew
       # Whether a given formula should be built from source during the current
       # installation run.
       def build_formula_from_source?(f)
-        return false if !build_from_source && !build_bottle
+        return false if !build_from_source? && !build_bottle?
 
         formulae.any? { |args_f| args_f.full_name == f.full_name }
       end
 
-      def build_from_source
-        return argv.include?("--build-from-source") || argv.include?("-s") unless args_parsed
-
-        build_from_source? || s?
-      end
-
-      def build_bottle
-        return argv.include?("--build-bottle") unless args_parsed
-
-        build_bottle?
-      end
-
-      def force_bottle
-        return argv.include?("--force-bottle") unless args_parsed
-
-        force_bottle?
-      end
-
       private
 
+      def option_to_name(option)
+        option.sub(/\A--?/, "")
+              .tr("-", "_")
+      end
+
+      def cli_args
+        return @cli_args if @cli_args
+
+        @cli_args = []
+        @processed_options.each do |short, long|
+          option = long || short
+          switch = "#{option_to_name(option)}?".to_sym
+          flag = option_to_name(option).to_sym
+          if @table[switch] == true || @table[flag] == true
+            @cli_args << option
+          elsif @table[flag].instance_of? String
+            @cli_args << option + "=" + @table[flag]
+          elsif @table[flag].instance_of? Array
+            @cli_args << option + "=" + @table[flag].join(",")
+          end
+        end
+        @cli_args.freeze
+      end
+
       def downcased_unique_named
         # Only lowercase names, not paths, bottle filenames or URLs
-        arguments = if args_parsed
+        named.map do |arg|
-          named
-        else
-          argv.reject { |arg| arg.start_with?("-") }
-        end
-        arguments.map do |arg|
           if arg.include?("/") || arg.end_with?(".tar.gz") || File.exist?(arg)
             arg
           else
@@ -209,28 +205,10 @@ module Homebrew
         end.uniq
       end
 
-      def head
-        return argv.include?("--HEAD") unless args_parsed
-
-        HEAD?
-      end
-
-      def devel
-        return argv.include?("--devel") unless args_parsed
-
-        devel?
-      end
-
-      def build_universal
-        return argv.include?("--universal") unless args_parsed
-
-        universal?
-      end
-
       def spec(default = :stable)
-        if head
+        if HEAD?
           :head
-        elsif devel
+        elsif devel?
           :devel
         else
           default

Library/Homebrew/cli/parser.rb

@@ -12,8 +12,8 @@ module Homebrew
     class Parser
       attr_reader :processed_options, :hide_from_man_page
 
-      def self.parse(args = ARGV, allow_no_named_args: false, &block)
+      def self.parse(argv = ARGV.dup.freeze, allow_no_named_args: false, &block)
-        new(args, &block).parse(args, allow_no_named_args: allow_no_named_args)
+        new(argv, &block).parse(allow_no_named_args: allow_no_named_args)
       end
 
       def self.from_cmd_path(cmd_path)
@@ -37,9 +37,10 @@ module Homebrew
         }
       end
 
-      def initialize(&block)
+      def initialize(argv = ARGV.dup.freeze, &block)
         @parser = OptionParser.new
-        @args = Homebrew::CLI::Args.new
+        @argv = argv
+        @args = Homebrew::CLI::Args.new(@argv)
 
         @constraints = []
         @conflicts = []
@@ -152,7 +153,7 @@ module Homebrew
         @parser.to_s
       end
 
-      def parse(argv = ARGV, allow_no_named_args: false)
+      def parse(argv = @argv, allow_no_named_args: false)
         raise "Arguments were already parsed!" if @args_parsed
 
         begin
@@ -161,12 +162,14 @@ module Homebrew
           $stderr.puts generate_help_text
           raise e
         end
+
         check_constraint_violations
         check_named_args(named_args, allow_no_named_args: allow_no_named_args)
-        @args[:remaining] = named_args
+        @args.freeze_named_args!(named_args)
+        parse_formula_options
         @args.freeze_processed_options!(@processed_options)
         Homebrew.args = @args
-        argv.freeze
+
         @args_parsed = true
         @parser
       end
@@ -186,21 +189,7 @@ module Homebrew
       end
 
       def formula_options
-        @args.formulae.each do |f|
+        @parse_formula_options = true
-          next if f.options.empty?
-
-          f.options.each do |o|
-            name = o.flag
-            description = "`#{f.name}`: #{o.description}"
-            if name.end_with? "="
-              flag   name, description: description
-            else
-              switch name, description: description
-            end
-          end
-        end
-      rescue FormulaUnavailableError
-        []
       end
 
       def max_named(count)
@@ -239,6 +228,26 @@ module Homebrew
 
       private
 
+      def parse_formula_options
+        return unless @parse_formula_options
+
+        @args.formulae.each do |f|
+          next if f.options.empty?
+
+          f.options.each do |o|
+            name = o.flag
+            description = "`#{f.name}`: #{o.description}"
+            if name.end_with? "="
+              flag   name, description: description
+            else
+              switch name, description: description
+            end
+          end
+        end
+      rescue FormulaUnavailableError
+        []
+      end
+
       def enable_switch(*names, from:)
         names.each do |name|
           @switch_sources[option_to_name(name)] = from

Library/Homebrew/cmd/install.rb

@@ -152,7 +152,7 @@ module Homebrew
       end
 
       # --HEAD, fail with no head defined
-      raise "No head is defined for #{f.full_name}" if args.head? && f.head.nil?
+      raise "No head is defined for #{f.full_name}" if args.HEAD? && f.head.nil?
 
       # --devel, fail with no devel defined
       raise "No devel block is defined for #{f.full_name}" if args.devel? && f.devel.nil?

Library/Homebrew/dev-cmd/irb.rb

@@ -18,7 +18,8 @@ module Homebrew
   module_function
 
   def irb_args
-    Homebrew::CLI::Parser.new do
+    # work around IRB modifying ARGV.
+    Homebrew::CLI::Parser.new(ARGV.dup) do
       usage_banner <<~EOS
         `irb` [<options>]
 
@@ -33,8 +34,7 @@ module Homebrew
   end
 
   def irb
-    # work around IRB modifying ARGV.
+    irb_args.parse
-    irb_args.parse(ARGV.dup)
 
     if args.examples?
       puts "'v8'.f # => instance of the v8 formula"

Library/Homebrew/extend/ENV/shared.rb

@@ -268,7 +268,7 @@ module SharedEnvExtension
 
   # @private
   def effective_arch
-    if Homebrew.args.build_bottle && ARGV.bottle_arch
+    if Homebrew.args.build_bottle? && ARGV.bottle_arch
       ARGV.bottle_arch
     else
       Hardware.oldest_cpu

Library/Homebrew/fetch.rb

@@ -5,7 +5,7 @@ module Homebrew
     module_function
 
     def fetch_bottle?(f)
-      return true if Homebrew.args.force_bottle && f.bottle
+      return true if Homebrew.args.force_bottle? && f.bottle
       return false unless f.bottle && f.pour_bottle?
       return false if Homebrew.args.build_formula_from_source?(f)
       return false unless f.bottle.compatible_cellar?

Library/Homebrew/formula_installer.rb

@@ -49,9 +49,9 @@ class FormulaInstaller
     @show_header = false
     @ignore_deps = false
     @only_deps = false
-    @build_from_source = Homebrew.args.build_from_source
+    @build_from_source = Homebrew.args.build_from_source?
     @build_bottle = false
-    @force_bottle = Homebrew.args.force_bottle
+    @force_bottle = Homebrew.args.force_bottle?
     @include_test = ARGV.include?("--include-test")
     @interactive = false
     @git = false

Library/Homebrew/global.rb

@@ -85,7 +85,7 @@ module Homebrew
     end
 
     def args
-      @args ||= CLI::Args.new
+      @args ||= CLI::Args.new(set_default_args: true)
     end
 
     def messages

Library/Homebrew/reinstall.rb

@@ -25,7 +25,7 @@ module Homebrew
 
     fi = FormulaInstaller.new(f)
     fi.options              = options
-    fi.build_bottle         = Homebrew.args.build_bottle
+    fi.build_bottle         = Homebrew.args.build_bottle?
     fi.interactive          = Homebrew.args.interactive?
     fi.git                  = Homebrew.args.git?
     fi.link_keg           ||= keg_was_linked if keg_had_linked_opt

Library/Homebrew/software_spec.rb

@@ -95,7 +95,7 @@ class SoftwareSpec
 
   def bottled?
     bottle_specification.tag?(Utils::Bottles.tag) && \
-      (bottle_specification.compatible_cellar? || Homebrew.args.force_bottle)
+      (bottle_specification.compatible_cellar? || Homebrew.args.force_bottle?)
   end
 
   def bottle(disable_type = nil, disable_reason = nil, &block)