Merge pull request #2242 from GauthamGoli/audit_custom_cops

Port audit_desc audit rules to a cop

Library/.rubocop.yml

@@ -11,6 +11,9 @@ require: ./Homebrew/rubocops.rb
 Homebrew/CorrectBottleBlock:
   Enabled: true
 
+Homebrew/FormulaDesc:
+  Enabled: true
+
 Metrics/AbcSize:
   Enabled: false
 

Library/Homebrew/dev-cmd/audit.rb

@@ -586,39 +586,6 @@ class FormulaAuditor
     problem "New formulae should not use `deprecated_option`."
   end
 
-  def audit_desc
-    # For now, only check the description when using `--strict`
-    return unless @strict
-
-    desc = formula.desc
-
-    unless desc && !desc.empty?
-      problem "Formula should have a desc (Description)."
-      return
-    end
-
-    # Make sure the formula name plus description is no longer than 80 characters
-    # Note full_name includes the name of the tap, while name does not
-    linelength = "#{formula.name}: #{desc}".length
-    if linelength > 80
-      problem <<-EOS.undent
-        Description is too long. \"name: desc\" should be less than 80 characters.
-        Length is calculated as #{formula.name} + desc. (currently #{linelength})
-      EOS
-    end
-
-    if desc =~ /([Cc]ommand ?line)/
-      problem "Description should use \"command-line\" instead of \"#{$1}\""
-    end
-
-    if desc =~ /^([Aa]n?)\s/
-      problem "Description shouldn't start with an indefinite article (#{$1})"
-    end
-
-    return unless desc.downcase.start_with? "#{formula.name} "
-    problem "Description shouldn't include the formula name"
-  end
-
   def audit_homepage
     homepage = formula.homepage
 
@@ -1281,7 +1248,6 @@ class FormulaAuditor
     audit_class
     audit_specs
     audit_revision_and_version_scheme
-    audit_desc
     audit_homepage
     audit_bottle_spec
     audit_github_repository

Library/Homebrew/rubocops.rb

@@ -1 +1,2 @@
 require_relative "./rubocops/bottle_block_cop"
+require_relative "./rubocops/formula_desc_cop"

Library/Homebrew/rubocops/bottle_block_cop.rb

@@ -1,16 +1,19 @@
+require_relative "./extend/formula_cop"
+
 module RuboCop
   module Cop
     module Homebrew
-      class CorrectBottleBlock < Cop
+      # This cop audits `bottle` block in Formulae
+      #
+      # - `rebuild` should be used instead of `revision` in `bottle` block
+
+      class CorrectBottleBlock < FormulaCop
         MSG = "Use rebuild instead of revision in bottle block".freeze
 
-        def on_block(node)
+        def audit_formula(_node, _class_node, _parent_class_node, formula_class_body_node)
-          return if block_length(node).zero?
+          bottle = find_block(formula_class_body_node, :bottle)
-          method, _args, body = *node
+          return if bottle.nil? || block_size(bottle).zero?
-          _keyword, method_name = *method
+          problem "Use rebuild instead of revision in bottle block" if method_called?(bottle, :revision)
-
-          return unless method_name == :bottle
-          check_revision?(body)
         end
 
         private
@@ -22,14 +25,6 @@ module RuboCop
             corrector.remove(node.source_range)
           end
         end
-
-        def check_revision?(body)
-          body.children.each do |method_call_node|
-            _receiver, method_name, _args = *method_call_node
-            next unless method_name == :revision
-            add_offense(method_call_node, :expression)
-          end
-        end
       end
     end
   end

Library/Homebrew/rubocops/extend/formula_cop.rb

@@ -0,0 +1,144 @@
+module RuboCop
+  module Cop
+    module Homebrew
+      class FormulaCop < Cop
+        @registry = Cop.registry
+
+        def on_class(node)
+          # This method is called by RuboCop and is the main entry point
+          file_path = processed_source.buffer.name
+          return unless file_path_allowed?(file_path)
+          class_node, parent_class_node, body = *node
+          return unless formula_class?(parent_class_node)
+          return unless respond_to?(:audit_formula)
+          @formula_name = class_name(class_node)
+          audit_formula(node, class_node, parent_class_node, body)
+        end
+
+        def regex_match_group(node, pattern)
+          # Checks for regex match of pattern in the node and
+          # Sets the appropriate instance variables to report the match
+          string_repr = string_content(node)
+          match_object = string_repr.match(pattern)
+          return unless match_object
+          node_begin_pos = start_column(node)
+          line_begin_pos = line_start_column(node)
+          @column = node_begin_pos + match_object.begin(0) - line_begin_pos + 1
+          @length = match_object.to_s.length
+          @line_no = line_number(node)
+          @source_buf = source_buffer(node)
+          @offense_source_range = source_range(@source_buf, @line_no, @column, @length)
+          @offensive_node = node
+          match_object
+        end
+
+        def find_node_method_by_name(node, method_name)
+          # Returns method_node matching method_name
+          return if node.nil?
+          node.each_child_node(:send) do |method_node|
+            next unless method_node.method_name == method_name
+            @offensive_node = method_node
+            @offense_source_range = method_node.source_range
+            return method_node
+          end
+          # If not found then, parent node becomes the offensive node
+          @offensive_node = node.parent
+          @offense_source_range = node.parent.source_range
+          nil
+        end
+
+        def find_block(node, block_name)
+          # Returns a block named block_name inside node
+          return if node.nil?
+          node.each_child_node(:block) do |block_node|
+            next if block_node.method_name != block_name
+            @offensive_node = block_node
+            @offense_source_range = block_node.source_range
+            return block_node
+          end
+          # If not found then, parent node becomes the offensive node
+          @offensive_node = node.parent
+          @offense_source_range = node.parent.source_range
+          nil
+        end
+
+        def method_called?(node, method_name)
+          # Check if a method is called inside a block
+          block_body = node.children[2]
+          block_body.each_child_node(:send) do |call_node|
+            next unless call_node.method_name == method_name
+            @offensive_node = call_node
+            @offense_source_range = call_node.source_range
+            return true
+          end
+          false
+        end
+
+        def parameters(method_node)
+          # Returns the array of arguments of the method_node
+          return unless method_node.send_type?
+          method_node.method_args
+        end
+
+        def line_start_column(node)
+          # Returns the begin position of the node's line in source code
+          node.source_range.source_buffer.line_range(node.loc.line).begin_pos
+        end
+
+        def start_column(node)
+          # Returns the begin position of the node in source code
+          node.source_range.begin_pos
+        end
+
+        def line_number(node)
+          # Returns the line number of the node
+          node.loc.line
+        end
+
+        def class_name(node)
+          # Returns the class node's name, nil if not a class node
+          @offensive_node = node
+          @offense_source_range = node.source_range
+          node.const_name
+        end
+
+        def size(node)
+          # Returns the node size in the source code
+          node.source_range.size
+        end
+
+        def block_size(block)
+          # Returns the block length of the block node
+          block_length(block)
+        end
+
+        def source_buffer(node)
+          # Source buffer is required as an argument to report style violations
+          node.source_range.source_buffer
+        end
+
+        def string_content(node)
+          # Returns the string representation if node is of type str
+          node.str_content if node.type == :str
+        end
+
+        def problem(msg)
+          add_offense(@offensive_node, @offense_source_range, msg)
+        end
+
+        private
+
+        def formula_class?(parent_class_node)
+          parent_class_node && parent_class_node.const_name == "Formula"
+        end
+
+        def file_path_allowed?(file_path)
+          paths_to_exclude = [%r{/Library/Homebrew/compat/},
+                              %r{/Library/Homebrew/test/}]
+          return true if file_path.nil? # file_path is nil when source is directly passed to the cop eg., in specs
+          file_path !~ Regexp.union(paths_to_exclude)
+        end
+      end
+    end
+  end
+end

Library/Homebrew/rubocops/formula_desc_cop.rb

@@ -0,0 +1,48 @@
+require_relative "./extend/formula_cop"
+require_relative "../extend/string"
+
+module RuboCop
+  module Cop
+    module Homebrew
+      # This cop audits `desc` in Formulae
+      #
+      # - Checks for existence of `desc`
+      # - Checks if size of `desc` > 80
+      # - Checks if `desc` begins with an article
+      # - Checks for correct usage of `command-line` in `desc`
+      # - Checks if `desc` contains the formula name
+      class FormulaDesc < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, body)
+          desc_call = find_node_method_by_name(body, :desc)
+
+          if desc_call.nil?
+            problem "Formula should have a desc (Description)."
+            return
+          end
+
+          desc = parameters(desc_call).first
+          desc_length = "#{@formula_name}: #{string_content(desc)}".length
+          max_desc_length = 80
+          if desc_length > max_desc_length
+            problem <<-EOS.undent
+              Description is too long. "name: desc" should be less than #{max_desc_length} characters.
+              Length is calculated as #{@formula_name} + desc. (currently #{desc_length})
+            EOS
+          end
+
+          # Check if command-line is wrongly used in formula's desc
+          if match = regex_match_group(desc, /(command ?line)/i)
+            problem "Description should use \"command-line\" instead of \"#{match}\""
+          end
+
+          if match = regex_match_group(desc, /^(an?)\s/i)
+            problem "Description shouldn't start with an indefinite article (#{match})"
+          end
+
+          # Check if formula's name is used in formula's desc
+          problem "Description shouldn't include the formula name" if regex_match_group(desc, /^#{@formula_name}/i)
+        end
+      end
+    end
+  end
+end

Library/Homebrew/test/Gemfile

@@ -2,6 +2,7 @@ source "https://rubygems.org"
 
 gem "parallel_tests"
 gem "rspec"
+gem "rubocop"
 gem "rspec-its", require: false
 gem "rspec-wait", require: false
 

Library/Homebrew/test/Gemfile.lock

@@ -1,6 +1,7 @@
 GEM
   remote: https://rubygems.org/
   specs:
+    ast (2.3.0)
     codecov (0.1.9)
       json
       simplecov
@@ -11,6 +12,10 @@ GEM
     parallel (1.10.0)
     parallel_tests (2.13.0)
       parallel
+    parser (2.4.0.0)
+      ast (~> 2.2)
+    powerpack (0.1.1)
+    rainbow (2.2.1)
     rspec (3.5.0)
       rspec-core (~> 3.5.0)
       rspec-expectations (~> 3.5.0)
@@ -29,11 +34,19 @@ GEM
     rspec-support (3.5.0)
     rspec-wait (0.0.9)
       rspec (>= 3, < 4)
+    rubocop (0.47.1)
+      parser (>= 2.3.3.1, < 3.0)
+      powerpack (~> 0.1)
+      rainbow (>= 1.99.1, < 3.0)
+      ruby-progressbar (~> 1.7)
+      unicode-display_width (~> 1.0, >= 1.0.1)
+    ruby-progressbar (1.8.1)
     simplecov (0.13.0)
       docile (~> 1.1.0)
       json (>= 1.8, < 3)
       simplecov-html (~> 0.10.0)
     simplecov-html (0.10.0)
+    unicode-display_width (1.1.3)
     url (0.3.2)
 
 PLATFORMS
@@ -45,6 +58,7 @@ DEPENDENCIES
   rspec
   rspec-its
   rspec-wait
+  rubocop
   simplecov
 
 BUNDLED WITH

Library/Homebrew/test/dev-cmd/audit_spec.rb

@@ -344,10 +344,6 @@ describe FormulaAuditor do
         end
       EOS
 
-      fa.audit_desc
-      expect(fa.problems.shift)
-        .to eq("Description shouldn't include the formula name")
-
       fa.audit_line 'ohai "#{share}/foolibc++"', 3
       expect(fa.problems.shift)
         .to eq("Use \#{pkgshare} instead of \#{share}/foolibc++")
@@ -413,32 +409,6 @@ describe FormulaAuditor do
       .to eq(["Don't recommend setuid in the caveats, suggest sudo instead."])
   end
 
-  specify "#audit_desc" do
-    formula_descriptions = [
-      { name: "foo", desc: nil,
-        problem: "Formula should have a desc" },
-      { name: "bar", desc: "bar" * 30,
-        problem: "Description is too long" },
-      { name: "baz", desc: "Baz commandline tool",
-        problem: "Description should use \"command-line\"" },
-      { name: "qux", desc: "A tool called Qux",
-        problem: "Description shouldn't start with an indefinite article" },
-    ]
-
-    formula_descriptions.each do |formula|
-      content = <<-EOS.undent
-        class #{Formulary.class_s(formula[:name])} < Formula
-          url "http://example.com/#{formula[:name]}-1.0.tgz"
-          desc "#{formula[:desc]}"
-        end
-      EOS
-
-      fa = formula_auditor formula[:name], content, strict: true
-      fa.audit_desc
-      expect(fa.problems.first).to match(formula[:problem])
-    end
-  end
-
   describe "#audit_homepage" do
     specify "homepage URLs" do
       fa = formula_auditor "foo", <<-EOS.undent, online: true

Library/Homebrew/test/rubocops/bottle_block_cop_spec.rb

@@ -0,0 +1,67 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/bottle_block_cop"
+
+describe RuboCop::Cop::Homebrew::CorrectBottleBlock do
+  subject(:cop) { described_class.new }
+
+  context "When auditing Bottle Block" do
+    it "When there is revision in bottle block" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          bottle do
+            cellar :any
+            revision 2
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "Use rebuild instead of revision in bottle block",
+                              severity: :convention,
+                              line: 5,
+                              column: 4,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    def expect_offense(expected, actual)
+      expect(actual.message).to eq(expected[:message])
+      expect(actual.severity).to eq(expected[:severity])
+      expect(actual.line).to eq(expected[:line])
+      expect(actual.column).to eq(expected[:column])
+    end
+  end
+
+  context "When auditing Bottle Block with auto correct" do
+    it "When there is revision in bottle block" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          bottle do
+            cellar :any
+            revision 2
+          end
+        end
+      EOS
+      corrected_source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          bottle do
+            cellar :any
+            rebuild 2
+          end
+        end
+      EOS
+
+      new_source = autocorrect_source(cop, source)
+      expect(new_source).to eq(corrected_source)
+    end
+  end
+end

Library/Homebrew/test/rubocops/formula_desc_cop_spec.rb

@@ -0,0 +1,121 @@
+require "rubocop"
+require "rubocop/rspec/support"
+require_relative "../../extend/string"
+require_relative "../../rubocops/formula_desc_cop"
+
+describe RuboCop::Cop::Homebrew::FormulaDesc do
+  subject(:cop) { described_class.new }
+
+  context "When auditing formula desc" do
+    it "When there is no desc" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+        end
+      EOS
+
+      expected_offenses = [{  message: "Formula should have a desc (Description).",
+                              severity: :convention,
+                              line: 1,
+                              column: 0,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When desc is too long" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          desc '#{"bar"*30}'
+        end
+      EOS
+
+      msg = <<-EOS.undent
+        Description is too long. "name: desc" should be less than 80 characters.
+        Length is calculated as Foo + desc. (currently 95)
+      EOS
+      expected_offenses = [{ message: msg,
+                             severity: :convention,
+                             line: 3,
+                             column: 2,
+                             source: source }]
+
+      inspect_source(cop, source)
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When wrong \"command-line\" usage in desc" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          desc 'command line'
+        end
+      EOS
+
+      expected_offenses = [{ message: "Description should use \"command-line\" instead of \"command line\"",
+                             severity: :convention,
+                             line: 3,
+                             column: 8,
+                             source: source }]
+
+      inspect_source(cop, source)
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When an article is used in desc" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          desc 'An '
+        end
+      EOS
+
+      expected_offenses = [{ message: "Description shouldn't start with an indefinite article (An )",
+                             severity: :convention,
+                             line: 3,
+                             column: 8,
+                             source: source }]
+
+      inspect_source(cop, source)
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "When formula name is in desc" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          url 'http://example.com/foo-1.0.tgz'
+          desc 'Foo'
+        end
+      EOS
+
+      expected_offenses = [{ message: "Description shouldn't include the formula name",
+                             severity: :convention,
+                             line: 3,
+                             column: 8,
+                             source: source }]
+
+      inspect_source(cop, source)
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    def expect_offense(expected, actual)
+      expect(actual.message).to eq(expected[:message])
+      expect(actual.severity).to eq(expected[:severity])
+      expect(actual.line).to eq(expected[:line])
+      expect(actual.column).to eq(expected[:column])
+    end
+  end
+end