Merge pull request #2995 from GauthamGoli/audit_line_rubocop_part_2

audit: Port line_problems to rubocop and add tests part 2

Library/Homebrew/dev-cmd/audit.rb

@@ -808,39 +808,6 @@ class FormulaAuditor
   end
 
   def line_problems(line, _lineno)
-    if line =~ /<(Formula|AmazonWebServicesFormula|ScriptFileFormula|GithubGistFormula)/
-      problem "Use a space in class inheritance: class Foo < #{Regexp.last_match(1)}"
-    end
-
-    # Commented-out cmake support from default template
-    problem "Commented cmake call found" if line.include?('# system "cmake')
-
-    # Comments from default template
-    [
-      "# PLEASE REMOVE",
-      "# Documentation:",
-      "# if this fails, try separate make/make install steps",
-      "# The URL of the archive",
-      "## Naming --",
-      "# if your formula requires any X11/XQuartz components",
-      "# if your formula fails when building in parallel",
-      "# Remove unrecognized options if warned by configure",
-    ].each do |comment|
-      next unless line.include?(comment)
-      problem "Please remove default template comments"
-    end
-
-    # FileUtils is included in Formula
-    # encfs modifies a file with this name, so check for some leading characters
-    if line =~ %r{[^'"/]FileUtils\.(\w+)}
-      problem "Don't need 'FileUtils.' before #{Regexp.last_match(1)}."
-    end
-
-    # Check for long inreplace block vars
-    if line =~ /inreplace .* do \|(.{2,})\|/
-      problem "\"inreplace <filenames> do |s|\" is preferred over \"|#{Regexp.last_match(1)}|\"."
-    end
-
     # Check for string interpolation of single values.
     if line =~ /(system|inreplace|gsub!|change_make_var!).*[ ,]"#\{([\w.]+)\}"/
       problem "Don't need to interpolate \"#{Regexp.last_match(2)}\" with #{Regexp.last_match(1)}"
@@ -890,9 +857,6 @@ class FormulaAuditor
       end
     end
 
-    # Commented-out depends_on
-    problem "Commented-out dep #{Regexp.last_match(1)}" if line =~ /#\s*depends_on\s+(.+)\s*$/
-
     if line =~ /if\s+ARGV\.include\?\s+'--(HEAD|devel)'/
       problem "Use \"if build.#{Regexp.last_match(1).downcase}?\" instead"
     end

Library/Homebrew/rubocops/extend/formula_cop.rb

@@ -86,9 +86,13 @@ module RuboCop
       end
 
       # Returns an array of method call nodes matching method_name in every descendant of node
-      def find_every_method_call_by_name(node, method_name)
+      # Returns every method call if no method_name is passed
+      def find_every_method_call_by_name(node, method_name = nil)
         return if node.nil?
-        node.each_descendant(:send).select { |method_node| method_name == method_node.method_name }
+        node.each_descendant(:send).select do |method_node|
+          method_name.nil? ||
+            method_name == method_node.method_name
+        end
       end
 
       # Given a method_name and arguments, yields to a block with
@@ -108,7 +112,7 @@ module RuboCop
       def find_instance_method_call(node, instance, method_name)
         methods = find_every_method_call_by_name(node, method_name)
         methods.each do |method|
-          next unless method.receiver.const_name == instance
+          next unless method.receiver && method.receiver.const_name == instance
           @offense_source_range = method.source_range
           @offensive_node = method
           yield method
@@ -202,9 +206,15 @@ module RuboCop
       end
 
       # Returns an array of block nodes of any depth below node in AST
+      # If a block is given then yields matching block node to the block!
       def find_all_blocks(node, block_name)
         return if node.nil?
-        node.each_descendant(:block).select { |block_node| block_name == block_node.method_name }
+        blocks = node.each_descendant(:block).select { |block_node| block_name == block_node.method_name }
+        return blocks unless block_given?
+        blocks.each do |block_node|
+          offending_node(block_node)
+          yield block_node
+        end
       end
 
       # Returns a method definition node with method_name
@@ -316,6 +326,15 @@ module RuboCop
         end
       end
 
+      # Yields to a block with comment text as parameter
+      def audit_comments
+        @processed_source.comments.each do |comment_node|
+          @offensive_node = comment_node
+          @offense_source_range = :expression
+          yield comment_node.text
+        end
+      end
+
       # Returns the begin position of the node's line in source code
       def line_start_column(node)
         node.source_range.source_buffer.line_range(node.loc.line).begin_pos
@@ -326,6 +345,11 @@ module RuboCop
         node.source_range.begin_pos
       end
 
+      # Returns the ending position of the node in source code
+      def end_column(node)
+        node.source_range.end_pos
+      end
+
       # Returns the line number of the node
       def line_number(node)
         node.loc.line

Library/Homebrew/rubocops/lines_cop.rb

@@ -15,6 +15,60 @@ module RuboCop
           problem ":tex is deprecated" if depends_on?(:tex)
         end
       end
+
+      class ClassInheritance < FormulaCop
+        def audit_formula(_node, class_node, parent_class_node, _body_node)
+          begin_pos = start_column(parent_class_node)
+          end_pos = end_column(class_node)
+          return unless begin_pos-end_pos != 3
+          problem "Use a space in class inheritance: class #{@formula_name} < #{class_name(parent_class_node)}"
+        end
+      end
+
+      class Comments < FormulaCop
+        def audit_formula(_node, _class_node, _parent_class_node, _body_node)
+          audit_comments do |comment|
+            [
+              "# PLEASE REMOVE",
+              "# Documentation:",
+              "# if this fails, try separate make/make install steps",
+              "# The URL of the archive",
+              "## Naming --",
+              "# if your formula requires any X11/XQuartz components",
+              "# if your formula fails when building in parallel",
+              "# Remove unrecognized options if warned by configure",
+              '# system "cmake',
+            ].each do |template_comment|
+              next unless comment.include?(template_comment)
+              problem "Please remove default template comments"
+              break
+            end
+          end
+
+          audit_comments do |comment|
+            # Commented-out depends_on
+            next unless comment =~ /#\s*depends_on\s+(.+)\s*$/
+            problem "Commented-out dependency #{Regexp.last_match(1)}"
+          end
+        end
+      end
+
+      class Miscellaneous < FormulaCop
+        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
+          find_instance_method_call(body_node, "FileUtils", nil) do |method_node|
+            problem "Don't need 'FileUtils.' before #{method_node.method_name}"
+          end
+
+          # Check for long inreplace block vars
+          find_all_blocks(body_node, :inreplace) do |node|
+            block_arg = node.arguments.children.first
+            next unless block_arg.source.size>1
+            problem "\"inreplace <filenames> do |s|\" is preferred over \"|#{block_arg.source}|\"."
+          end
+        end
+      end
     end
   end
 end

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

@@ -263,28 +263,6 @@ describe FormulaAuditor do
       expect(fa.problems.shift)
         .to eq('Use pkgshare instead of (share/"foolibc++")')
     end
-
-    specify "no space in class inheritance" do
-      fa = formula_auditor "foo", <<-EOS.undent
-        class Foo<Formula
-          url '/foo-1.0.tgz'
-        end
-      EOS
-
-      fa.line_problems "class Foo<Formula", 1
-      expect(fa.problems.shift)
-        .to eq("Use a space in class inheritance: class Foo < Formula")
-    end
-
-    specify "default template" do
-      fa = formula_auditor "foo", "class Foo < Formula; url '/foo-1.0.tgz'; end"
-
-      fa.line_problems '# system "cmake", ".", *std_cmake_args', 3
-      expect(fa.problems.shift).to eq("Commented cmake call found")
-
-      fa.line_problems "# PLEASE REMOVE", 3
-      expect(fa.problems.shift).to eq("Please remove default template comments")
-    end
   end
 
   describe "#audit_github_repository" do

Library/Homebrew/test/rubocops/lines_cop_spec.rb

@@ -51,3 +51,154 @@ describe RuboCop::Cop::FormulaAudit::Lines do
     end
   end
 end
+
+describe RuboCop::Cop::FormulaAudit::ClassInheritance do
+  subject(:cop) { described_class.new }
+
+  context "When auditing lines" do
+    it "with no space in class inheritance" do
+      source = <<-EOS.undent
+        class Foo<Formula
+          desc "foo"
+          url 'http://example.com/foo-1.0.tgz'
+        end
+      EOS
+
+      expected_offenses = [{  message: "Use a space in class inheritance: class Foo < Formula",
+                              severity: :convention,
+                              line: 1,
+                              column: 10,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+  end
+end
+
+describe RuboCop::Cop::FormulaAudit::Comments do
+  subject(:cop) { described_class.new }
+
+  context "When auditing formula" do
+    it "with commented cmake call" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          desc "foo"
+          url 'http://example.com/foo-1.0.tgz'
+          # system "cmake", ".", *std_cmake_args
+        end
+      EOS
+
+      expected_offenses = [{  message: "Please remove default template comments",
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "with default template comments" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          # PLEASE REMOVE
+          desc "foo"
+          url 'http://example.com/foo-1.0.tgz'
+        end
+      EOS
+
+      expected_offenses = [{  message: "Please remove default template comments",
+                              severity: :convention,
+                              line: 2,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "with commented out depends_on" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          desc "foo"
+          url 'http://example.com/foo-1.0.tgz'
+          # depends_on "foo"
+        end
+      EOS
+
+      expected_offenses = [{  message: 'Commented-out dependency "foo"',
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+  end
+end
+
+describe RuboCop::Cop::FormulaAudit::Miscellaneous do
+  subject(:cop) { described_class.new }
+
+  context "When auditing formula" do
+    it "with FileUtils" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          desc "foo"
+          url 'http://example.com/foo-1.0.tgz'
+          FileUtils.mv "hello"
+        end
+      EOS
+
+      expected_offenses = [{  message: "Don't need 'FileUtils.' before mv",
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+
+    it "with long inreplace block vars" do
+      source = <<-EOS.undent
+        class Foo < Formula
+          desc "foo"
+          url 'http://example.com/foo-1.0.tgz'
+          inreplace "foo" do |longvar|
+            somerandomCall(longvar)
+          end
+        end
+      EOS
+
+      expected_offenses = [{  message: "\"inreplace <filenames> do |s|\" is preferred over \"|longvar|\".",
+                              severity: :convention,
+                              line: 4,
+                              column: 2,
+                              source: source }]
+
+      inspect_source(cop, source)
+
+      expected_offenses.zip(cop.offenses).each do |expected, actual|
+        expect_offense(expected, actual)
+      end
+    end
+  end
+end