From bd6e9e72a1d50b58e338eca543f34d07fbaad853 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Thu, 13 Apr 2023 19:10:38 +0100 Subject: [PATCH] rubocops/cask: Check for correct stanza ordering within `on_*` blocks - Now that we detect correct stanza _grouping_ within `on_*` blocks in Casks (PR 15211), correct stanza _ordering_ in `on_*` blocks was the next logical step. For example, `url` has to come after `version` and `sha256` in an `on_macos` or `on_intel` block for consistency with the top-level stanza order we enforce elsewhere. - Still not doing the nested `on_os` inside `on_arch`, that felt excessive for an edge case that isn't present in any actual real Casks we have. I removed the test with that specific TODO. --- .../Homebrew/rubocops/cask/ast/cask_block.rb | 4 + .../Homebrew/rubocops/cask/stanza_order.rb | 30 +++-- .../test/rubocops/cask/stanza_order_spec.rb | 120 ++++++++---------- 3 files changed, 76 insertions(+), 78 deletions(-) diff --git a/Library/Homebrew/rubocops/cask/ast/cask_block.rb b/Library/Homebrew/rubocops/cask/ast/cask_block.rb index 1b4090ab49..929a4aeb67 100644 --- a/Library/Homebrew/rubocops/cask/ast/cask_block.rb +++ b/Library/Homebrew/rubocops/cask/ast/cask_block.rb @@ -51,6 +51,10 @@ module RuboCop @sorted_toplevel_stanzas ||= sort_stanzas(toplevel_stanzas) end + def sorted_inner_stanzas(stanzas) + sort_stanzas(stanzas) + end + private def sort_stanzas(stanzas) diff --git a/Library/Homebrew/rubocops/cask/stanza_order.rb b/Library/Homebrew/rubocops/cask/stanza_order.rb index 83264020bb..6423fc613b 100644 --- a/Library/Homebrew/rubocops/cask/stanza_order.rb +++ b/Library/Homebrew/rubocops/cask/stanza_order.rb @@ -13,11 +13,23 @@ module RuboCop extend AutoCorrector include CaskHelp + ON_SYSTEM_METHODS = RuboCop::Cask::Constants::ON_SYSTEM_METHODS MESSAGE = "`%s` stanza out of order" def on_cask(cask_block) @cask_block = cask_block - add_offenses + add_offenses(toplevel_stanzas) + + return unless (on_blocks = toplevel_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, inner: true) + end end private @@ -25,22 +37,24 @@ module RuboCop attr_reader :cask_block def_delegators :cask_block, :cask_node, :toplevel_stanzas, - :sorted_toplevel_stanzas + :sorted_toplevel_stanzas, :sorted_inner_stanzas - def add_offenses - offending_stanzas.each do |stanza| + def add_offenses(stanzas, inner: false) + sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas + offending_stanzas(stanzas, inner: inner).each do |stanza| message = format(MESSAGE, stanza: stanza.stanza_name) add_offense(stanza.source_range_with_comments, message: message) do |corrector| - correct_stanza_index = toplevel_stanzas.index(stanza) - correct_stanza = sorted_toplevel_stanzas[correct_stanza_index] + correct_stanza_index = stanzas.index(stanza) + correct_stanza = sorted_stanzas[correct_stanza_index] corrector.replace(stanza.source_range_with_comments, correct_stanza.source_with_comments) end end end - def offending_stanzas - stanza_pairs = toplevel_stanzas.zip(sorted_toplevel_stanzas) + def offending_stanzas(stanzas, inner: false) + sorted_stanzas = inner ? sorted_inner_stanzas(stanzas) : sorted_toplevel_stanzas + stanza_pairs = stanzas.zip(sorted_stanzas) stanza_pairs.each_with_object([]) do |stanza_pair, offending_stanzas| stanza, sorted_stanza = *stanza_pair offending_stanzas << stanza if stanza != sorted_stanza diff --git a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb index b7357cd744..b1da650dfe 100644 --- a/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb +++ b/Library/Homebrew/test/rubocops/cask/stanza_order_spec.rb @@ -458,39 +458,22 @@ describe RuboCop::Cop::Cask::StanzaOrder do include_examples "autocorrects source" end - # TODO: detect out-of-order stanzas in nested expressions - context "when stanzas are nested in a conditional expression" do - let(:source) do - <<~CASK - cask 'foo' do - if true - sha256 :no_check - version :latest - end - end - CASK - end - - include_examples "does not report any offenses" - end - context "when `on_arch` blocks are out of order" do let(:source) do <<~CASK cask 'foo' do on_intel do + version :latest + sha256 :no_check + url "https://foo.brew.sh/foo-intel.zip" - sha256 :no_check - version :latest end - on_arm do - url "https://foo.brew.sh/foo-arm.zip" - sha256 :no_check version :latest - end + sha256 :no_check - name "Foo" + url "https://foo.brew.sh/foo-arm.zip" + end end CASK end @@ -501,13 +484,13 @@ describe RuboCop::Cop::Cask::StanzaOrder do severity: :convention, line: 2, column: 2, - source: "on_intel do\n url \"https://foo.brew.sh/foo-intel.zip\"\n sha256 :no_check\n version :latest\n end", # rubocop:disable Layout/LineLength + source: "on_intel do\n version :latest\n sha256 :no_check\n\n url \"https://foo.brew.sh/foo-intel.zip\"\n end", # rubocop:disable Layout/LineLength }, { message: "Cask/StanzaOrder: `on_arm` stanza out of order", severity: :convention, line: 8, column: 2, - source: "on_arm do\n url \"https://foo.brew.sh/foo-arm.zip\"\n sha256 :no_check\n version :latest\n end", # rubocop:disable Layout/LineLength + source: "on_arm do\n version :latest\n sha256 :no_check\n\n url \"https://foo.brew.sh/foo-arm.zip\"\n end", # rubocop:disable Layout/LineLength }] end @@ -515,18 +498,17 @@ describe RuboCop::Cop::Cask::StanzaOrder do <<~CASK cask 'foo' do on_arm do + version :latest + sha256 :no_check + url "https://foo.brew.sh/foo-arm.zip" - sha256 :no_check - version :latest end - on_intel do - url "https://foo.brew.sh/foo-intel.zip" - sha256 :no_check version :latest - end + sha256 :no_check - name "Foo" + url "https://foo.brew.sh/foo-intel.zip" + end end CASK end @@ -535,44 +517,42 @@ describe RuboCop::Cop::Cask::StanzaOrder do include_examples "autocorrects source" end - # TODO: detect out-of-order stanzas in nested expressions - context "when the on_arch and on_os stanzas are nested" do + context "when the `on_arch` blocks contents are out of order" do let(:source) do <<~CASK cask 'foo' do on_arm do - url "https://foo.brew.sh/foo-arm-all.zip" - sha256 :no_check - version :latest + url "https://foo.brew.sh/foo-arm.zip" + sha256 "123abc" + version "1.0" end - on_intel do - on_ventura do - url "https://foo.brew.sh/foo-intel-ventura.zip" - sha256 :no_check - end - on_mojave do - url "https://foo.brew.sh/foo-intel-mojave.zip" - sha256 :no_check - end - on_catalina do - url "https://foo.brew.sh/foo-intel-catalina.zip" - sha256 :no_check - end - on_big_sur do - url "https://foo.brew.sh/foo-intel-big-sur.zip" - sha256 :no_check - end - - version :latest + url "https://foo.brew.sh/foo-intel.zip" + sha256 "abc123" + version "0.9" # comment here end - - name "Foo" end CASK end - include_examples "does not report any offenses" + let(:correct_source) do + <<~CASK + cask 'foo' do + on_arm do + version "1.0" + sha256 "123abc" + url "https://foo.brew.sh/foo-arm.zip" + end + on_intel do + version "0.9" # comment here + sha256 "abc123" + url "https://foo.brew.sh/foo-intel.zip" + end + end + CASK + end + + include_examples "autocorrects source" end context "when the on_os stanzas are out of order" do @@ -580,20 +560,20 @@ describe RuboCop::Cop::Cask::StanzaOrder do <<~CASK cask "foo" do on_ventura do - url "https://foo.brew.sh/foo-ventura.zip" sha256 :no_check + url "https://foo.brew.sh/foo-ventura.zip" end on_catalina do - url "https://foo.brew.sh/foo-catalina.zip" sha256 :no_check + url "https://foo.brew.sh/foo-catalina.zip" end on_mojave do - url "https://foo.brew.sh/foo-mojave.zip" sha256 :no_check + url "https://foo.brew.sh/foo-mojave.zip" end on_big_sur do - url "https://foo.brew.sh/foo-big-sur.zip" sha256 :no_check + url "https://foo.brew.sh/foo-big-sur.zip" end name "Foo" @@ -607,19 +587,19 @@ describe RuboCop::Cop::Cask::StanzaOrder do severity: :convention, line: 2, column: 2, - source: "on_ventura do\n url \"https://foo.brew.sh/foo-ventura.zip\"\n sha256 :no_check\n end", + source: "on_ventura do\n sha256 :no_check\n url \"https://foo.brew.sh/foo-ventura.zip\"\n end", }, { message: "Cask/StanzaOrder: `on_mojave` stanza out of order", severity: :convention, line: 10, column: 2, - source: "on_mojave do\n url \"https://foo.brew.sh/foo-mojave.zip\"\n sha256 :no_check\n end", + source: "on_mojave do\n sha256 :no_check\n url \"https://foo.brew.sh/foo-mojave.zip\"\n end", }, { message: "Cask/StanzaOrder: `on_big_sur` stanza out of order", severity: :convention, line: 14, column: 2, - source: "on_big_sur do\n url \"https://foo.brew.sh/foo-big-sur.zip\"\n sha256 :no_check\n end", + source: "on_big_sur do\n sha256 :no_check\n url \"https://foo.brew.sh/foo-big-sur.zip\"\n end", }] end @@ -627,20 +607,20 @@ describe RuboCop::Cop::Cask::StanzaOrder do <<~CASK cask "foo" do on_mojave do - url "https://foo.brew.sh/foo-mojave.zip" sha256 :no_check + url "https://foo.brew.sh/foo-mojave.zip" end on_catalina do - url "https://foo.brew.sh/foo-catalina.zip" sha256 :no_check + url "https://foo.brew.sh/foo-catalina.zip" end on_big_sur do - url "https://foo.brew.sh/foo-big-sur.zip" sha256 :no_check + url "https://foo.brew.sh/foo-big-sur.zip" end on_ventura do - url "https://foo.brew.sh/foo-ventura.zip" sha256 :no_check + url "https://foo.brew.sh/foo-ventura.zip" end name "Foo"