Merge pull request #6534 from reitermarkus/style

Refactor `brew style` and `brew cask style`.

Library/Homebrew/cask/cmd/style.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "json"
+
 module Cask
   class Cmd
     class Style < AbstractCommand
@@ -7,26 +9,50 @@ module Cask
         "checks Cask style using RuboCop"
       end
 
+      def self.rubocop(*paths, auto_correct: false, debug: false, json: false)
+        Homebrew.install_bundler_gems!
+
+        cache_env = { "XDG_CACHE_HOME" => "#{HOMEBREW_CACHE}/style" }
+        hide_warnings = debug ? [] : [ENV["HOMEBREW_RUBY_PATH"], "-W0", "-S"]
+
+        args = [
+          "--force-exclusion",
+          "--config", "#{HOMEBREW_LIBRARY}/.rubocop_cask.yml"
+        ]
+
+        if json
+          args << "--format" << "json"
+        else
+          if auto_correct
+            args << "--auto-correct"
+          else
+            args << "--debug" if debug
+            args << "--parallel"
+          end
+
+          args << "--format" << "simple"
+          args << "--color" if Tty.color?
+        end
+
+        executable, *args = [*hide_warnings, "rubocop", *args, "--", *paths]
+
+        result = Dir.mktmpdir do |tmpdir|
+          system_command executable, args: args, chdir: tmpdir, env: cache_env,
+                                     print_stdout: !json, print_stderr: !json
+        end
+
+        result.assert_success! unless (0..1).cover?(result.exit_status)
+
+        return JSON.parse(result.stdout) if json
+
+        result
+      end
+
       option "--fix", :fix, false
 
       def run
-        install_rubocop
+        result = self.class.rubocop(*cask_paths, auto_correct: fix?, debug: debug?)
-        cache_env = { "XDG_CACHE_HOME" => "#{HOMEBREW_CACHE}/style" }
+        raise CaskError, "Style check failed." unless result.status.success?
-        hide_warnings = debug? ? [] : [ENV["HOMEBREW_RUBY_PATH"], "-W0", "-S"]
-        Dir.mktmpdir do |tmpdir|
-          system(cache_env, *hide_warnings, "rubocop", *rubocop_args, "--", *cask_paths, chdir: tmpdir)
-        end
-        raise CaskError, "style check failed" unless $CHILD_STATUS.success?
-      end
-
-      def install_rubocop
-        capture_stderr do
-          begin
-            Homebrew.install_bundler_gems!
-          rescue SystemExit
-            raise CaskError, Tty.strip_ansi($stderr.string).chomp.sub(/\AError: /, "")
-          end
-        end
       end
 
       def cask_paths
@@ -39,32 +65,12 @@ module Cask
         end
       end
 
-      def rubocop_args
-        fix? ? autocorrect_args : normal_args
-      end
-
-      def default_args
-        [
-          "--force-exclusion",
-          "--config", "#{HOMEBREW_LIBRARY}/.rubocop_cask.yml",
-          "--format", "simple"
-        ]
-      end
-
       def test_cask_paths
         [
           Pathname.new("#{HOMEBREW_LIBRARY}/Homebrew/test/support/fixtures/cask/Casks"),
           Pathname.new("#{HOMEBREW_LIBRARY}/Homebrew/test/support/fixtures/third-party/Casks"),
         ]
       end
-
-      def normal_args
-        default_args + ["--parallel"]
-      end
-
-      def autocorrect_args
-        default_args + ["--auto-correct"]
-      end
     end
   end
 end

Library/Homebrew/cmd/style.rb

@@ -62,6 +62,6 @@ module Homebrew
                                  NewFormulaAudit]
     end
 
-    Homebrew.failed = Style.check_style_and_print(target, options)
+    Homebrew.failed = !Style.check_style_and_print(target, options)
   end
 end

Library/Homebrew/dev-cmd/audit.rb

@@ -95,7 +95,7 @@ module Homebrew
       odie "--only-cops/--except-cops and --strict/--only cannot be used simultaneously!"
     end
 
-    options = { fix: args.fix?, realpath: true }
+    options = { fix: args.fix? }
 
     if only_cops
       options[:only_cops] = only_cops

Library/Homebrew/style.rb

@@ -6,19 +6,17 @@ module Homebrew
 
     # Checks style for a list of files, printing simple RuboCop output.
     # Returns true if violations were found, false otherwise.
-    def check_style_and_print(files, options = {})
+    def check_style_and_print(files, **options)
-      check_style_impl(files, :print, options)
+      check_style_impl(files, :print, **options)
     end
 
     # Checks style for a list of files, returning results as a RubocopResults
     # object parsed from its JSON output.
-    def check_style_json(files, options = {})
+    def check_style_json(files, **options)
-      check_style_impl(files, :json, options)
+      check_style_impl(files, :json, **options)
     end
 
-    def check_style_impl(files, output_type, options = {})
+    def check_style_impl(files, output_type, fix: false, except_cops: nil, only_cops: nil)
-      fix = options[:fix]
-
       Homebrew.install_bundler_gems!
       require "rubocop"
       require "rubocops"
@@ -34,22 +32,22 @@ module Homebrew
 
       args += ["--extra-details", "--display-cop-names"] if ARGV.verbose?
 
-      if options[:except_cops]
+      if except_cops
-        options[:except_cops].map! { |cop| RuboCop::Cop::Cop.registry.qualified_cop_name(cop.to_s, "") }
+        except_cops.map! { |cop| RuboCop::Cop::Cop.registry.qualified_cop_name(cop.to_s, "") }
-        cops_to_exclude = options[:except_cops].select do |cop|
+        cops_to_exclude = except_cops.select do |cop|
           RuboCop::Cop::Cop.registry.names.include?(cop) ||
             RuboCop::Cop::Cop.registry.departments.include?(cop.to_sym)
         end
 
         args << "--except" << cops_to_exclude.join(",") unless cops_to_exclude.empty?
-      elsif options[:only_cops]
+      elsif only_cops
-        options[:only_cops].map! { |cop| RuboCop::Cop::Cop.registry.qualified_cop_name(cop.to_s, "") }
+        only_cops.map! { |cop| RuboCop::Cop::Cop.registry.qualified_cop_name(cop.to_s, "") }
-        cops_to_include = options[:only_cops].select do |cop|
+        cops_to_include = only_cops.select do |cop|
           RuboCop::Cop::Cop.registry.names.include?(cop) ||
             RuboCop::Cop::Cop.registry.departments.include?(cop.to_sym)
         end
 
-        odie "RuboCops #{options[:only_cops].join(",")} were not found" if cops_to_include.empty?
+        odie "RuboCops #{only_cops.join(",")} were not found" if cops_to_include.empty?
 
         args << "--only" << cops_to_include.join(",")
       end
@@ -102,7 +100,7 @@ module Homebrew
         raise "Invalid output_type for check_style_impl: #{output_type}"
       end
 
-      return !rubocop_success if files.present? || !has_non_formula
+      return rubocop_success if files.present? || !has_non_formula
 
       shellcheck   = which("shellcheck")
       shellcheck ||= which("shellcheck", ENV["HOMEBREW_PATH"])
@@ -113,7 +111,7 @@ module Homebrew
       end
       unless shellcheck
         opoo "Could not find or install `shellcheck`! Not checking shell style."
-        return !rubocop_success
+        return rubocop_success
       end
 
       shell_files = [
@@ -125,7 +123,7 @@ module Homebrew
       # TODO: check, fix completions here too.
       # TODO: consider using ShellCheck JSON output
       shellcheck_success = system shellcheck, "--shell=bash", *shell_files
-      !rubocop_success || !shellcheck_success
+      rubocop_success && shellcheck_success
     end
 
     class RubocopResults
@@ -168,8 +166,8 @@ module Homebrew
         "[Corrected] " if corrected?
       end
 
-      def to_s(options = {})
+      def to_s(display_cop_name: false)
-        if options[:display_cop_name]
+        if display_cop_name
           "#{severity_code}: #{location.to_short_s}: #{cop_name}: " \
           "#{Tty.green}#{correction_status}#{Tty.reset}#{message}"
         else

Library/Homebrew/system_command.rb

@@ -49,7 +49,8 @@ class SystemCommand
       end
     end
 
-    assert_success if must_succeed?
+    result = Result.new(command, @output, @status, secrets: @secrets)
+    result.assert_success! if must_succeed?
     result
   end
 
@@ -105,12 +106,6 @@ class SystemCommand
     ["/usr/bin/sudo", *askpass_flags, "-E", "--"]
   end
 
-  def assert_success
-    return if @status.success?
-
-    raise ErrorDuringExecution.new(command, status: @status, output: @output, secrets: @secrets)
-  end
-
   def expanded_args
     @expanded_args ||= args.map do |arg|
       if arg.respond_to?(:to_path)
@@ -166,18 +161,21 @@ class SystemCommand
     sources.each(&:close_read)
   end
 
-  def result
-    Result.new(command, @output, @status)
-  end
-
   class Result
     attr_accessor :command, :status, :exit_status
 
-    def initialize(command, output, status)
+    def initialize(command, output, status, secrets:)
       @command       = command
       @output        = output
       @status        = status
       @exit_status   = status.exitstatus
+      @secrets       = secrets
+    end
+
+    def assert_success!
+      return if @status.success?
+
+      raise ErrorDuringExecution.new(command, status: @status, output: @output, secrets: @secrets)
     end
 
     def stdout

Library/Homebrew/test/cask/cmd/style_spec.rb

@@ -11,58 +11,6 @@ describe Cask::Cmd::Style, :cask do
 
   it_behaves_like "a command that handles invalid options"
 
-  describe "#run" do
-    subject { cli.run }
-
-    before do
-      allow(cli).to receive_messages(install_rubocop: nil,
-                                     system:          nil,
-                                     rubocop_args:    nil,
-                                     cask_paths:      nil)
-      allow($CHILD_STATUS).to receive(:success?).and_return(success)
-    end
-
-    context "when rubocop succeeds" do
-      let(:success) { true }
-
-      it "does not raise an error" do
-        expect { subject }.not_to raise_error
-      end
-    end
-
-    context "when rubocop fails" do
-      let(:success) { false }
-
-      it "raises an error" do
-        expect { subject }.to raise_error(Cask::CaskError)
-      end
-    end
-  end
-
-  describe "#install_rubocop" do
-    subject { cli.install_rubocop }
-
-    context "when installation succeeds" do
-      before do
-        allow(Homebrew).to receive(:install_bundler_gems!)
-      end
-
-      it "exits successfully" do
-        expect { subject }.not_to raise_error
-      end
-    end
-
-    context "when installation fails" do
-      before do
-        allow(Homebrew).to receive(:install_bundler_gems!).and_raise(SystemExit)
-      end
-
-      it "raises an error" do
-        expect { subject }.to raise_error(Cask::CaskError)
-      end
-    end
-  end
-
   describe "#cask_paths" do
     subject { cli.cask_paths }
 
@@ -117,41 +65,4 @@ describe Cask::Cmd::Style, :cask do
       end
     end
   end
-
-  describe "#rubocop_args" do
-    subject { cli.rubocop_args }
-
-    before do
-      allow(cli).to receive(:fix?).and_return(fix)
-    end
-
-    context "when fix? is true" do
-      let(:fix) { true }
-
-      it { is_expected.to include("--auto-correct") }
-    end
-
-    context "when fix? is false" do
-      let(:fix) { false }
-
-      it { is_expected.not_to include("--auto-correct") }
-    end
-  end
-
-  describe "#default_args" do
-    subject { cli.default_args }
-
-    it { is_expected.to include("--format", "simple") }
-  end
-
-  describe "#autocorrect_args" do
-    subject { cli.autocorrect_args }
-
-    let(:default_args) { ["--format", "simple"] }
-
-    it "adds --auto-correct to default args" do
-      allow(cli).to receive(:default_args).and_return(default_args)
-      expect(subject).to include("--auto-correct", *default_args)
-    end
-  end
 end

Library/Homebrew/test/cask/pkg_spec.rb

@@ -152,7 +152,8 @@ describe Cask::Pkg, :cask do
         "/usr/sbin/pkgutil",
         args: ["--pkg-info-plist", pkg_id],
       ).and_return(
-        SystemCommand::Result.new(nil, [[:stdout, pkg_info_plist]], instance_double(Process::Status, exitstatus: 0)),
+        SystemCommand::Result.new(nil, [[:stdout, pkg_info_plist]], instance_double(Process::Status, exitstatus: 0),
+                                  secrets: []),
       )
 
       info = pkg.info

Library/Homebrew/test/cmd/style_spec.rb

@@ -62,8 +62,10 @@ describe "brew style" do
           end
         end
       EOS
-      options = { fix: true, only_cops: ["NewFormulaAudit/DependencyOrder"], realpath: true }
+      rubocop_result = Homebrew::Style.check_style_json(
-      rubocop_result = Homebrew::Style.check_style_json([formula], options)
+        [formula],
+        fix: true, only_cops: ["NewFormulaAudit/DependencyOrder"],
+      )
       offense_string = rubocop_result.file_offenses(formula.realpath).first.to_s
       expect(offense_string).to match(/\[Corrected\]/)
     end
@@ -79,7 +81,7 @@ describe "brew style" do
 
       rubocop_result = Homebrew::Style.check_style_and_print([target_file])
 
-      expect(rubocop_result).to eq false
+      expect(rubocop_result).to eq true
     end
   end
 end

Library/Homebrew/test/support/helper/cask/fake_system_command.rb

@@ -55,7 +55,7 @@ class FakeSystemCommand
     if response.respond_to?(:call)
       response.call(command_string, options)
     else
-      SystemCommand::Result.new(command, [[:stdout, response]], OpenStruct.new(exitstatus: 0))
+      SystemCommand::Result.new(command, [[:stdout, response]], OpenStruct.new(exitstatus: 0), secrets: [])
     end
   end
 

Library/Homebrew/test/system_command_result_spec.rb

@@ -4,7 +4,8 @@ require "system_command"
 
 describe SystemCommand::Result do
   subject(:result) {
-    described_class.new([], output_array, instance_double(Process::Status, exitstatus: 0, success?: true))
+    described_class.new([], output_array, instance_double(Process::Status, exitstatus: 0, success?: true),
+                        secrets: [])
   }
 
   let(:output_array) {

Library/Homebrew/utils/gems.rb

@@ -92,7 +92,7 @@ module Homebrew
     @bundle_installed ||= begin
       bundle = "#{gem_user_bindir}/bundle"
       bundle_check_output = `#{bundle} check 2>&1`
-      bundle_check_failed = !$CHILD_STATUS.exitstatus.zero?
+      bundle_check_failed = !$CHILD_STATUS.success?
 
       # for some reason sometimes the exit code lies so check the output too.
       if bundle_check_failed || bundle_check_output.include?("Install missing gems")

Library/Homebrew/utils/tty.rb

@@ -64,10 +64,17 @@ module Tty
   end
 
   def to_s
-    return "" if !ENV["HOMEBREW_COLOR"] && (ENV["HOMEBREW_NO_COLOR"] || !$stdout.tty?)
+    return "" unless color?
 
     current_escape_sequence
   ensure
     reset_escape_sequence!
   end
+
+  def color?
+    return false if ENV["HOMEBREW_NO_COLOR"]
+    return true if ENV["HOMEBREW_COLOR"]
+
+    $stdout.tty?
+  end
 end