Merge pull request #7348 from MikeMcQuaid/brew-style-fixes

`brew style` fixes

Library/.rubocop.yml

@@ -8,10 +8,6 @@ FormulaAudit:
 FormulaAuditStrict:
   Enabled: true
 
-# disable all formulae strict audits by default
-NewFormulaAudit:
-  Enabled: false
-
 # make our hashes consistent
 Layout/HashAlignment:
   EnforcedHashRocketStyle: table

Library/.rubocop_audit.yml

-inherit_from: ./.rubocop.yml
-
-NewFormulaAudit:
-  Enabled: true

Library/.rubocop_rspec.yml

@@ -8,9 +8,6 @@ AllCops:
   Exclude:
     - '**/vendor/**/*'
 
-NewFormulaAudit:
-  Enabled: true
-
 # Intentionally disabled as it doesn't fit with our code style.
 RSpec/AnyInstance:
   Enabled: false

Library/Homebrew/cmd/style.rb

@@ -57,9 +57,7 @@ module Homebrew
     elsif except_cops
       options[:except_cops] = except_cops
     elsif only_cops.nil? && except_cops.nil?
-      options[:except_cops] = %w[FormulaAudit
-                                 FormulaAuditStrict
-                                 NewFormulaAudit]
+      options[:except_cops] = %w[FormulaAuditStrict]
     end
 
     Homebrew.failed = !Style.check_style_and_print(target, options)

Library/Homebrew/dev-cmd/audit.rb

@@ -95,8 +95,6 @@ module Homebrew
       options[:only_cops] = only_cops
     elsif args.new_formula?
       nil
-    elsif strict
-      options[:except_cops] = [:NewFormulaAudit]
     elsif except_cops
       options[:except_cops] = except_cops
     elsif !strict
@@ -231,12 +229,6 @@ module Homebrew
       return unless @style_offenses
 
       @style_offenses.each do |offense|
-        if offense.cop_name.start_with?("NewFormulaAudit")
-          next if @versioned_formula
-
-          new_formula_problem offense.to_s(display_cop_name: @display_cop_names)
-          next
-        end
         problem offense.to_s(display_cop_name: @display_cop_names)
       end
     end

Library/Homebrew/rubocops/class.rb

@@ -26,11 +26,18 @@ module RuboCop
         end
       end
 
-      class TestCalls < FormulaCop
+      class Test < FormulaCop
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           test = find_block(body_node, :test)
           return unless test
 
+          if test.body.nil?
+            problem "`test do` should not be empty"
+            return
+          end
+
+          problem "`test do` should contain a real test" if test.body.single_line? && test.body.source.to_s == "true"
+
           test_calls(test) do |node, params|
             p1, p2 = params
             if match = string_content(p1).match(%r{(/usr/local/(s?bin))})
@@ -70,26 +77,13 @@ module RuboCop
       end
     end
 
-    module FormulaAudit
-      # - `test do ..end` should be meaningfully defined in the formula.
-      class Test < FormulaCop
+    module FormulaAuditStrict
+      # - `test do ..end` should defined in the formula.
+      class TestPresent < FormulaCop
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
-          test = find_block(body_node, :test)
-
-          unless test
-            problem "A `test do` test block should be added"
-            return
-          end
-
-          if test.body.nil?
-            problem "`test do` should not be empty"
-            return
-          end
-
-          return unless test.body.single_line?
-          return if test.body.source.to_s != "true"
+          return if find_block(body_node, :test)
 
-          problem "`test do` should contain a real test"
+          problem "A `test do` test block should be added"
         end
       end
     end

Library/Homebrew/rubocops/formula_desc.rb

@@ -10,37 +10,6 @@ module RuboCop
       #
       # - Checks for existence of `desc`
       # - Checks if size of `desc` > 80
-      class DescLength < FormulaCop
-        def audit_formula(_node, _class_node, _parent_class_node, body_node)
-          desc_call = find_node_method_by_name(body_node, :desc)
-
-          # Check if a formula's desc is present
-          if desc_call.nil?
-            problem "Formula should have a desc (Description)."
-            return
-          end
-
-          # Check the formula's desc length. Should be >0 and <80 characters.
-          desc = parameters(desc_call).first
-          pure_desc_length = string_content(desc).length
-          if pure_desc_length.zero?
-            problem "The desc (description) should not be an empty string."
-            return
-          end
-
-          desc_length = "#{@formula_name}: #{string_content(desc)}".length
-          max_desc_length = 80
-          return if desc_length <= max_desc_length
-
-          problem "Description is too long. \"name: desc\" should be less than #{max_desc_length} characters. " \
-                  "Length is calculated as #{@formula_name} + desc. (currently #{desc_length})"
-        end
-      end
-    end
-
-    module FormulaAudit
-      # This cop audits `desc` in Formulae.
-      #
       # - Checks for leading/trailing whitespace in `desc`
       # - Checks if `desc` begins with an article
       # - Checks for correct usage of `command-line` in `desc`
@@ -49,23 +18,27 @@ module RuboCop
       # - Checks if `desc` ends with a full stop (apart from in the case of "etc.")
       class Desc < FormulaCop
         VALID_LOWERCASE_WORDS = %w[
-          ex
-          eXtensible
-          iOS
           macOS
-          malloc
-          ooc
-          preexec
-          x86
-          xUnit
         ].freeze
 
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           desc_call = find_node_method_by_name(body_node, :desc)
-          return if desc_call.nil?
+
+          # Check if a formula's desc is present
+          if desc_call.nil?
+            problem "Formula should have a desc (Description)."
+            return
+          end
 
           desc = parameters(desc_call).first
 
+          # Check the formula's desc length. Should be >0 and <80 characters.
+          pure_desc_length = string_content(desc).length
+          if pure_desc_length.zero?
+            problem "The desc (description) should not be an empty string."
+            return
+          end
+
           # Check for leading whitespace.
           problem "Description shouldn't have a leading space" if regex_match_group(desc, /^\s+/)
 
@@ -96,9 +69,16 @@ module RuboCop
           end
 
           # Check if a full stop is used at the end of a formula's desc (apart from in the case of "etc.")
-          return unless regex_match_group(desc, /\.$/) && !string_content(desc).end_with?("etc.")
+          if regex_match_group(desc, /\.$/) && !string_content(desc).end_with?("etc.")
+            problem "Description shouldn't end with a full stop"
+          end
 
-          problem "Description shouldn't end with a full stop"
+          desc_length = "#{@formula_name}: #{string_content(desc)}".length
+          max_desc_length = 80
+          return if desc_length <= max_desc_length
+
+          problem "Description is too long. \"name: desc\" should be less than #{max_desc_length} characters. " \
+                  "Length is calculated as #{@formula_name} + desc. (currently #{desc_length})"
         end
 
         def autocorrect(node)

Library/Homebrew/rubocops/lines.rb

@@ -196,26 +196,6 @@ module RuboCop
       end
 
       class Miscellaneous < FormulaCop
-        MAKE_CHECK_WHITELIST = %w[
-          beecrypt
-          ccrypt
-          git
-          gmp
-          gnupg
-          gnupg@1.4
-          google-sparsehash
-          jemalloc
-          jpeg-turbo
-          mpfr
-          nettle
-          open-mpi
-          openssl@1.1
-          pcre
-          protobuf
-          wolfssl
-          xz
-        ].freeze
-
         def audit_formula(_node, _class_node, _parent_class_node, body_node)
           # FileUtils is included in Formula
           # encfs modifies a file with this name, so check for some leading characters
@@ -260,18 +240,18 @@ module RuboCop
           # Avoid hard-coding compilers
           find_every_method_call_by_name(body_node, :system).each do |method|
             param = parameters(method).first
-            if match = regex_match_group(param, %r{^(/usr/bin/)?(gcc|llvm-gcc|clang)\s?})
+            if match = regex_match_group(param, %r{^(/usr/bin/)?(gcc|llvm-gcc|clang)[\s"]?})
               problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{match[2]}\""
-            elsif match = regex_match_group(param, %r{^(/usr/bin/)?((g|llvm-g|clang)\+\+)\s?})
+            elsif match = regex_match_group(param, %r{^(/usr/bin/)?((g|llvm-g|clang)\+\+)[\s"]?})
               problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{match[2]}\""
             end
           end
 
           find_instance_method_call(body_node, "ENV", :[]=) do |method|
             param = parameters(method)[1]
-            if match = regex_match_group(param, %r{^(/usr/bin/)?(gcc|llvm-gcc|clang)\s?})
+            if match = regex_match_group(param, %r{^(/usr/bin/)?(gcc|llvm-gcc|clang)[\s"]?})
               problem "Use \"\#{ENV.cc}\" instead of hard-coding \"#{match[2]}\""
-            elsif match = regex_match_group(param, %r{^(/usr/bin/)?((g|llvm-g|clang)\+\+)\s?})
+            elsif match = regex_match_group(param, %r{^(/usr/bin/)?((g|llvm-g|clang)\+\+)[\s"]?})
               problem "Use \"\#{ENV.cxx}\" instead of hard-coding \"#{match[2]}\""
             end
           end
@@ -439,25 +419,6 @@ module RuboCop
 
             problem "Use the `#{match}` Ruby method instead of `#{method.source}`"
           end
-
-          return if formula_tap != "homebrew-core"
-
-          # Avoid build-time checks in homebrew/core
-          find_every_method_call_by_name(body_node, :system).each do |method|
-            next if @formula_name.start_with?("lib")
-            next if MAKE_CHECK_WHITELIST.include?(@formula_name)
-
-            params = parameters(method)
-            next unless node_equals?(params[0], "make")
-
-            params[1..].each do |arg|
-              next unless regex_match_group(arg, /^(checks?|tests?)$/)
-
-              offending_node(method)
-              problem "Formulae in homebrew/core (except e.g. cryptography, libraries) " \
-                      "should not run build-time checks"
-            end
-          end
         end
 
         def modifier?(node)
@@ -493,5 +454,50 @@ module RuboCop
         EOS
       end
     end
+
+    module FormulaAuditStrict
+      class MakeCheck < FormulaCop
+        MAKE_CHECK_WHITELIST = %w[
+          beecrypt
+          ccrypt
+          git
+          gmp
+          gnupg
+          gnupg@1.4
+          google-sparsehash
+          jemalloc
+          jpeg-turbo
+          mpfr
+          nettle
+          open-mpi
+          openssl@1.1
+          pcre
+          protobuf
+          wolfssl
+          xz
+        ].freeze
+
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          return if formula_tap != "homebrew-core"
+
+          # Avoid build-time checks in homebrew/core
+          find_every_method_call_by_name(body_node, :system).each do |method|
+            next if @formula_name.start_with?("lib")
+            next if MAKE_CHECK_WHITELIST.include?(@formula_name)
+
+            params = parameters(method)
+            next unless node_equals?(params[0], "make")
+
+            params[1..].each do |arg|
+              next unless regex_match_group(arg, /^(checks?|tests?)$/)
+
+              offending_node(method)
+              problem "Formulae in homebrew/core (except e.g. cryptography, libraries) " \
+                      "should not run build-time checks"
+            end
+          end
+        end
+      end
+    end
   end
 end

Library/Homebrew/rubocops/options.rb

@@ -46,12 +46,5 @@ module RuboCop
         end
       end
     end
-
-    # Keep this (empty) module and class around in case we need it later to
-    # avoid deleting all the NewFormulaAudit referencing logic.
-    module NewFormulaAudit
-      class Options < FormulaCop
-      end
-    end
   end
 end

Library/Homebrew/rubocops/patches.rb

@@ -44,7 +44,7 @@ module RuboCop
                                             %r{gist\.github\.com/.+/raw},
                                             %r{gist\.githubusercontent\.com/.+/raw}])
           if regex_match_group(patch, gh_patch_patterns)
-            unless patch_url.match?(/[a-fA-F0-9]{40}/)
+            unless patch_url.match?(%r{/[a-fA-F0-9]{6,40}/})
               problem <<~EOS.chomp
                 GitHub/Gist patches should specify a revision:
                   #{patch_url}

Library/Homebrew/rubocops/text.rb

@@ -61,18 +61,17 @@ module RuboCop
           find_method_with_args(body_node, :system, "cargo", "build") do
             problem "use \"cargo\", \"install\", \"--root\", prefix, \"--path\", \".\""
           end
-
-          find_method_with_args(body_node, :go_resource) do
-            problem "`go_resource`s are deprecated. Please ask upstream to implement Go vendoring"
-          end
         end
       end
     end
 
-    # Keep this (empty) module and class around in case we need it later to
-    # avoid deleting all the FormulaAuditStrict referencing logic.
     module FormulaAuditStrict
       class Text < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body_node)
+          find_method_with_args(body_node, :go_resource) do
+            problem "`go_resource`s are deprecated. Please ask upstream to implement Go vendoring"
+          end
+        end
       end
     end
   end

Library/Homebrew/style.rb

@@ -60,7 +60,7 @@ module Homebrew
         config = if files.first && File.exist?("#{files.first}/spec")
           HOMEBREW_LIBRARY/".rubocop_rspec.yml"
         else
-          HOMEBREW_LIBRARY/".rubocop_audit.yml"
+          HOMEBREW_LIBRARY/".rubocop.yml"
         end
         args << "--config" << config
       end

Library/Homebrew/test/.rubocop_todo.yml

@@ -50,6 +50,7 @@ RSpec/FilePath:
     - 'rubocops/patches_spec.rb'
     - 'rubocops/text_spec.rb'
     - 'rubocops/uses_from_macos_spec.rb'
+    - 'rubocops/formula_desc_spec.rb'
     - 'search_spec.rb'
     - 'string_spec.rb'
     - 'system_command_result_spec.rb'

Library/Homebrew/test/cmd/style_spec.rb

@@ -10,13 +10,13 @@ end
 describe "brew style" do
   around do |example|
     FileUtils.ln_s HOMEBREW_LIBRARY_PATH, HOMEBREW_LIBRARY/"Homebrew"
-    FileUtils.ln_s HOMEBREW_LIBRARY_PATH.parent/".rubocop.yml", HOMEBREW_LIBRARY/".rubocop_audit.yml"
+    FileUtils.ln_s HOMEBREW_LIBRARY_PATH.parent/".rubocop.yml", HOMEBREW_LIBRARY/".rubocop.yml"
     FileUtils.ln_s HOMEBREW_LIBRARY_PATH.parent/".rubocop_shared.yml", HOMEBREW_LIBRARY/".rubocop_shared.yml"
 
     example.run
   ensure
     FileUtils.rm_f HOMEBREW_LIBRARY/"Homebrew"
-    FileUtils.rm_f HOMEBREW_LIBRARY/".rubocop_audit.yml"
+    FileUtils.rm_f HOMEBREW_LIBRARY/".rubocop.yml"
     FileUtils.rm_f HOMEBREW_LIBRARY/".rubocop_shared.yml"
   end
 
@@ -62,7 +62,7 @@ describe "brew style" do
       EOS
       rubocop_result = Homebrew::Style.check_style_json(
         [formula],
-        fix: true, only_cops: ["NewFormulaAudit/DependencyOrder"],
+        fix: true, only_cops: ["FormulaAudit/DependencyOrder"],
       )
       offense_string = rubocop_result.file_offenses(formula.realpath).first.to_s
       expect(offense_string).to match(/\[Corrected\]/)

Library/Homebrew/test/rubocops/class_spec.rb

@@ -50,7 +50,7 @@ describe RuboCop::Cop::FormulaAudit::ClassName do
   end
 end
 
-describe RuboCop::Cop::FormulaAudit::TestCalls do
+describe RuboCop::Cop::FormulaAudit::Test do
   subject(:cop) { described_class.new }
 
   it "reports an offense when /usr/local/bin is found in test calls" do
@@ -79,65 +79,65 @@ describe RuboCop::Cop::FormulaAudit::TestCalls do
     RUBY
   end
 
-  it "supports auto-correcting test calls" do
-    source = <<~RUBY
+  it "reports an offense when there is an empty test block" do
+    expect_offense(<<~RUBY)
       class Foo < Formula
         url 'https://brew.sh/foo-1.0.tgz'
 
         test do
-          shell_output("/usr/local/sbin/test", 0)
+        ^^^^^^^ `test do` should not be empty
         end
       end
     RUBY
+  end
 
-    corrected_source = <<~RUBY
+  it "reports an offense when test is falsely true" do
+    expect_offense(<<~RUBY)
       class Foo < Formula
         url 'https://brew.sh/foo-1.0.tgz'
 
         test do
-          shell_output("\#{sbin}/test")
+        ^^^^^^^ `test do` should contain a real test
+          true
         end
       end
     RUBY
-
-    new_source = autocorrect_source(source)
-    expect(new_source).to eq(corrected_source)
   end
-end
-
-describe RuboCop::Cop::FormulaAudit::Test do
-  subject(:cop) { described_class.new }
 
-  it "reports an offense when there is no test block" do
-    expect_offense(<<~RUBY)
+  it "supports auto-correcting test calls" do
+    source = <<~RUBY
       class Foo < Formula
-      ^^^^^^^^^^^^^^^^^^^ A `test do` test block should be added
         url 'https://brew.sh/foo-1.0.tgz'
+
+        test do
+          shell_output("/usr/local/sbin/test", 0)
+        end
       end
     RUBY
-  end
 
-  it "reports an offense when there is an empty test block" do
-    expect_offense(<<~RUBY)
+    corrected_source = <<~RUBY
       class Foo < Formula
         url 'https://brew.sh/foo-1.0.tgz'
 
         test do
-        ^^^^^^^ `test do` should not be empty
+          shell_output("\#{sbin}/test")
         end
       end
     RUBY
+
+    new_source = autocorrect_source(source)
+    expect(new_source).to eq(corrected_source)
   end
+end
 
-  it "reports an offense when test is falsely true" do
+describe RuboCop::Cop::FormulaAuditStrict::TestPresent do
+  subject(:cop) { described_class.new }
+
+  it "reports an offense when there is no test block" do
     expect_offense(<<~RUBY)
       class Foo < Formula
+      ^^^^^^^^^^^^^^^^^^^ A `test do` test block should be added
         url 'https://brew.sh/foo-1.0.tgz'
-
-        test do
-        ^^^^^^^ `test do` should contain a real test
-          true
-        end
       end
     RUBY
   end

Library/Homebrew/test/rubocops/formula_desc_spec.rb

@@ -2,7 +2,7 @@
 
 require "rubocops/formula_desc"
 
-describe RuboCop::Cop::FormulaAudit::DescLength do
+describe RuboCop::Cop::FormulaAudit::Desc do
   subject(:cop) { described_class.new }
 
   context "When auditing formula desc" do
@@ -46,10 +46,6 @@ describe RuboCop::Cop::FormulaAudit::DescLength do
       RUBY
     end
   end
-end
-
-describe RuboCop::Cop::FormulaAudit::Desc do
-  subject(:cop) { described_class.new }
 
   context "When auditing formula desc" do
     it "When wrong \"command-line\" usage in desc" do

Library/Homebrew/test/rubocops/lines_spec.rb

@@ -349,17 +349,6 @@ describe RuboCop::Cop::FormulaAudit::Miscellaneous do
   subject(:cop) { described_class.new }
 
   context "When auditing formula" do
-    it "build-time checks in homebrew/core" do
-      expect_offense(<<~RUBY, "/homebrew-core/")
-        class Foo < Formula
-          desc "foo"
-          url 'https://brew.sh/foo-1.0.tgz'
-          system "make", "-j1", "test"
-          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Formulae in homebrew/core (except e.g. cryptography, libraries) should not run build-time checks
-        end
-      RUBY
-    end
-
     it "FileUtils usage" do
       expect_offense(<<~RUBY)
         class Foo < Formula
@@ -860,6 +849,21 @@ describe RuboCop::Cop::FormulaAudit::Miscellaneous do
       RUBY
     end
   end
+end
+
+describe RuboCop::Cop::FormulaAuditStrict::MakeCheck do
+  subject(:cop) { described_class.new }
+
+  it "build-time checks in homebrew/core" do
+    expect_offense(<<~RUBY, "/homebrew-core/")
+      class Foo < Formula
+        desc "foo"
+        url 'https://brew.sh/foo-1.0.tgz'
+        system "make", "-j1", "test"
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Formulae in homebrew/core (except e.g. cryptography, libraries) should not run build-time checks
+      end
+    RUBY
+  end
 
   include_examples "formulae exist", described_class::MAKE_CHECK_WHITELIST
 end