Merge pull request #15211 from issyl0/rubocop-cask-stanza-grouping-in-on-blocks

Library/Homebrew/rubocops/cask/ast/cask_block.rb

@@ -32,7 +32,7 @@ module RuboCop
 
           @stanzas ||= cask_body.each_node
                                 .select(&:stanza?)
-                                .map { |node| Stanza.new(node, stanza_comments(node)) }
+                                .map { |node| Stanza.new(node, comments) }
         end
 
         def toplevel_stanzas
@@ -68,17 +68,6 @@ module RuboCop
         def stanza_order_index(stanza)
           Constants::STANZA_ORDER.index(stanza.stanza_name)
         end
-
-        def stanza_comments(stanza_node)
-          stanza_node.each_node.reduce([]) do |comments, node|
-            comments | comments_hash[node.loc]
-          end
-        end
-
-        def comments_hash
-          @comments_hash ||= Parser::Source::Comment
-                             .associate_locations(cask_node, comments)
-        end
       end
     end
   end

Library/Homebrew/rubocops/cask/ast/stanza.rb

@@ -12,12 +12,12 @@ module RuboCop
       class Stanza
         extend Forwardable
 
-        def initialize(method_node, comments)
+        def initialize(method_node, all_comments)
           @method_node = method_node
-          @comments = comments
+          @all_comments = all_comments
         end
 
-        attr_reader :method_node, :comments
+        attr_reader :method_node, :all_comments
 
         alias stanza_node method_node
 
@@ -52,6 +52,16 @@ module RuboCop
           stanza_group == other.stanza_group
         end
 
+        def comments
+          @comments ||= stanza_node.each_node.reduce([]) do |comments, node|
+            comments | comments_hash[node.loc]
+          end
+        end
+
+        def comments_hash
+          @comments_hash ||= Parser::Source::Comment.associate_locations(stanza_node.parent, all_comments)
+        end
+
         def ==(other)
           self.class == other.class && stanza_node == other.stanza_node
         end

Library/Homebrew/rubocops/cask/stanza_grouping.rb

@@ -14,16 +14,28 @@ module RuboCop
         include CaskHelp
         include RangeHelp
 
-        MISSING_LINE_MSG = "stanza groups should be separated by a single " \
+        ON_SYSTEM_METHODS = RuboCop::Cask::Constants::ON_SYSTEM_METHODS
-                           "empty line"
+        MISSING_LINE_MSG = "stanza groups should be separated by a single empty line"
-
+        EXTRA_LINE_MSG = "stanzas within the same group should have no lines between them"
-        EXTRA_LINE_MSG = "stanzas within the same group should have no lines " \
-                         "between them"
 
         def on_cask(cask_block)
           @cask_block = cask_block
           @line_ops = {}
-          add_offenses
+          cask_stanzas = cask_block.toplevel_stanzas
+          add_offenses(cask_stanzas)
+
+          # If present, check grouping of stanzas within `on_*` blocks.
+          return unless (on_blocks = cask_stanzas.select { |s| ON_SYSTEM_METHODS.include?(s.stanza_name) }).any?
+
+          on_blocks.map(&:method_node).each do |on_block|
+            next unless on_block.block_type?
+
+            block_contents = on_block.child_nodes.select(&:begin_type?)
+            inner_nodes = block_contents.map(&:child_nodes).flatten.select(&:send_type?)
+            inner_stanzas = inner_nodes.map { |node| RuboCop::Cask::AST::Stanza.new(node, processed_source.comments) }
+
+            add_offenses(inner_stanzas)
+          end
         end
 
         private
@@ -32,8 +44,8 @@ module RuboCop
 
         def_delegators :cask_block, :cask_node, :toplevel_stanzas
 
-        def add_offenses
+        def add_offenses(stanzas)
-          toplevel_stanzas.each_cons(2) do |stanza, next_stanza|
+          stanzas.each_cons(2) do |stanza, next_stanza|
             next unless next_stanza
 
             if missing_line_after?(stanza, next_stanza)

Library/Homebrew/test/rubocops/cask/stanza_grouping_spec.rb

@@ -506,20 +506,111 @@ describe RuboCop::Cop::Cask::StanzaGrouping do
     include_examples "autocorrects source"
   end
 
-  # TODO: detect incorrectly grouped stanzas in nested expressions
+  context "when stanzas are nested one-level in `on_*` blocks" do
-  context "when stanzas are nested in a conditional expression" do
+    describe "basic nesting" do
-    let(:source) do
+      let(:source) do
-      <<~CASK
+        <<~CASK
-        cask 'foo' do
+          cask 'foo' do
-          if true
+            on_arm do
-            version :latest
+              version "1.0.2"
 
-            sha256 :no_check
+              sha256 :no_check
+            end
+            on_intel do
+              version "0.9.8"
+              sha256 :no_check
+              url "https://foo.brew.sh/foo-intel.zip"
+            end
           end
-        end
+        CASK
-      CASK
+      end
+
+      let(:correct_source) do
+        <<~CASK
+          cask 'foo' do
+            on_arm do
+              version "1.0.2"
+              sha256 :no_check
+            end
+            on_intel do
+              version "0.9.8"
+              sha256 :no_check
+
+              url "https://foo.brew.sh/foo-intel.zip"
+            end
+          end
+        CASK
+      end
+
+      include_examples "autocorrects source"
     end
 
-    include_examples "does not report any offenses"
+    describe "nested `on_*` blocks with comments" do
+      let(:source) do
+        <<~CASK
+          cask 'foo' do
+            on_arm do
+              version "1.0.2"
+
+              sha256 :no_check # comment on same line
+            end
+            on_intel do
+              version "0.9.8"
+              sha256 :no_check
+            end
+          end
+        CASK
+      end
+
+      let(:correct_source) do
+        <<~CASK
+          cask 'foo' do
+            on_arm do
+              version "1.0.2"
+              sha256 :no_check # comment on same line
+            end
+            on_intel do
+              version "0.9.8"
+              sha256 :no_check
+            end
+          end
+        CASK
+      end
+
+      include_examples "autocorrects source"
+    end
+
+    # TODO: Maybe this should be fixed too?
+    describe "inner erroneously grouped nested livecheck block contents are ignored" do
+      let(:source) do
+        <<~CASK
+          cask 'foo' do
+            on_arm do
+              version "1.0.2"
+              sha256 :no_check
+
+              url "https://foo.brew.sh/foo-arm.zip"
+
+              livecheck do
+                url "https://foo.brew.sh/foo-arm-versions.html"
+              end
+            end
+            on_intel do
+              version "0.9.8"
+              sha256 :no_check
+
+              url "https://foo.brew.sh/foo-intel.zip"
+
+              livecheck do
+                regex(/RegExhibit\s+(\d+(?:.\d+)+)/i)
+                url "https://foo.brew.sh/foo-intel-versions.html"
+              end
+            end
+          end
+        CASK
+      end
+
+      include_examples "does not report any offenses"
+    end
   end
 end