diff --git a/Library/Homebrew/rubocops/cask/constants/stanza.rb b/Library/Homebrew/rubocops/cask/constants/stanza.rb index 2fe782a17d..64c524d015 100644 --- a/Library/Homebrew/rubocops/cask/constants/stanza.rb +++ b/Library/Homebrew/rubocops/cask/constants/stanza.rb @@ -65,6 +65,20 @@ module RuboCop end.freeze STANZA_ORDER = STANZA_GROUPS.flatten.freeze + + UNINSTALL_METHODS_ORDER = [ + :early_script, + :launchctl, + :quit, + :signal, + :login_item, + :kext, + :script, + :pkgutil, + :delete, + :trash, + :rmdir, + ].freeze end end end diff --git a/Library/Homebrew/rubocops/cask/uninstall_methods_order.rb b/Library/Homebrew/rubocops/cask/uninstall_methods_order.rb new file mode 100644 index 0000000000..07c0216665 --- /dev/null +++ b/Library/Homebrew/rubocops/cask/uninstall_methods_order.rb @@ -0,0 +1,49 @@ +# typed: strict +# frozen_string_literal: true + +require "rubocops/shared/helper_functions" + +module RuboCop + module Cop + module Cask + # This cop checks for the correct order of methods within the 'uninstall' and 'zap' stanzas. + class UninstallMethodsOrder < Base + extend AutoCorrector + include CaskHelp + include HelperFunctions + + MSG = T.let("`%s` method out of order".freeze, String) + + sig { params(node: AST::SendNode).void } + def on_send(node) + return unless [:zap, :uninstall].include?(node.method_name) + + hash_node = node.arguments.first + return if hash_node.nil? || (!hash_node.is_a?(AST::Node) && !hash_node.hash_type?) + + method_nodes = hash_node.pairs.map(&:key) + expected_order = method_nodes.sort_by { |method| method_order_index(method) } + + method_nodes.each_with_index do |method, index| + next if method == expected_order[index] + + add_offense(method, message: format(MSG, method: method.children.first)) do |corrector| + indentation = " " * (start_column(method) - line_start_column(method)) + ordered_sources = expected_order.map do |expected_method| + hash_node.pairs.find { |pair| pair.key == expected_method }.source + end + new_code = ordered_sources.join(",\n#{indentation}") + corrector.replace(hash_node.source_range, new_code) + end + end + end + + sig { params(method_node: AST::SymbolNode).returns(Integer) } + def method_order_index(method_node) + method_name = method_node.children.first + RuboCop::Cask::Constants::UNINSTALL_METHODS_ORDER.index(method_name) || -1 + end + end + end + end +end diff --git a/Library/Homebrew/rubocops/rubocop-cask.rb b/Library/Homebrew/rubocops/rubocop-cask.rb index 279f2ad467..dd498fb261 100644 --- a/Library/Homebrew/rubocops/rubocop-cask.rb +++ b/Library/Homebrew/rubocops/rubocop-cask.rb @@ -18,6 +18,7 @@ require_relative "cask/discontinued" require_relative "cask/homepage_url_trailing_slash" require_relative "cask/no_overrides" require_relative "cask/on_system_conditionals" +require_relative "cask/uninstall_methods_order" require_relative "cask/stanza_order" require_relative "cask/stanza_grouping" require_relative "cask/url" diff --git a/Library/Homebrew/test/rubocops/cask/uninstall_methods_order_spec.rb b/Library/Homebrew/test/rubocops/cask/uninstall_methods_order_spec.rb new file mode 100644 index 0000000000..4e7c5180b7 --- /dev/null +++ b/Library/Homebrew/test/rubocops/cask/uninstall_methods_order_spec.rb @@ -0,0 +1,211 @@ +# frozen_string_literal: true + +require "rubocops/rubocop-cask" +describe RuboCop::Cop::Cask::UninstallMethodsOrder, :config do + context "with order errors in both the uninstall and zap block" do + it "reports an offense and corrects the order" do + expect_offense(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall delete: [ + ^^^^^^ `delete` method out of order + "/usr/local/bin/foo", + "/usr/local/bin/foobar", + ], + script: { + ^^^^^^ `script` method out of order + executable: "/usr/local/bin/foo", + sudo: false, + }, + pkgutil: "org.foo.bar" + ^^^^^^^ `pkgutil` method out of order + + zap delete: [ + "~/Library/Application Support/Bar", + "~/Library/Application Support/Foo", + ], + rmdir: "~/Library/Application Support", + ^^^^^ `rmdir` method out of order + trash: "~/Library/Application Support/FooBar" + ^^^^^ `trash` method out of order + end + CASK + + expect_correction(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall script: { + executable: "/usr/local/bin/foo", + sudo: false, + }, + pkgutil: "org.foo.bar", + delete: [ + "/usr/local/bin/foo", + "/usr/local/bin/foobar", + ] + + zap delete: [ + "~/Library/Application Support/Bar", + "~/Library/Application Support/Foo", + ], + trash: "~/Library/Application Support/FooBar", + rmdir: "~/Library/Application Support" + end + CASK + end + end + + context "with incorrectly ordered uninstall methods" do + it "reports an offense and corrects the order" do + expect_offense(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall delete: [ + ^^^^^^ `delete` method out of order + "/usr/local/bin/foo", + "/usr/local/bin/foobar", + ], + script: { + ^^^^^^ `script` method out of order + executable: "/usr/local/bin/foo", + sudo: false, + }, + pkgutil: "org.foo.bar" + ^^^^^^^ `pkgutil` method out of order + end + CASK + + expect_correction(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall script: { + executable: "/usr/local/bin/foo", + sudo: false, + }, + pkgutil: "org.foo.bar", + delete: [ + "/usr/local/bin/foo", + "/usr/local/bin/foobar", + ] + end + CASK + end + end + + context "with correctly ordered uninstall methods" do + it "does not report an offense" do + expect_no_offenses(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall script: { + executable: "/usr/local/bin/foo", + sudo: false, + }, + pkgutil: "org.foo.bar", + delete: [ + "/usr/local/bin/foo", + "/usr/local/bin/foobar", + ] + end + CASK + end + end + + context "with a single method in uninstall block" do + it "does not report an offense" do + expect_no_offenses(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall delete: "/usr/local/bin/foo" + end + CASK + expect_no_offenses(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + uninstall pkgutil: [ + "org.foo.bar", + "org.foobar.bar", + ] + end + CASK + end + end + + context "with incorrectly ordered zap methods" do + it "reports an offense and corrects the order" do + expect_offense(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + zap delete: [ + "~/Library/Application Support/Foo", + "~/Library/Application Support/Bar", + ], + rmdir: "~/Library/Application Support", + ^^^^^ `rmdir` method out of order + trash: "~/Library/Application Support/FooBar" + ^^^^^ `trash` method out of order + end + CASK + + expect_correction(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + zap delete: [ + "~/Library/Application Support/Foo", + "~/Library/Application Support/Bar", + ], + trash: "~/Library/Application Support/FooBar", + rmdir: "~/Library/Application Support" + end + CASK + end + end + + context "with correctly ordered zap methods" do + it "does not report an offense" do + expect_no_offenses(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + zap delete: [ + "~/Library/Application Support/Bar", + "~/Library/Application Support/Foo", + ], + trash: "~/Library/Application Support/FooBar", + rmdir: "~/Library/Application Support" + end + CASK + end + end + + context "with a single method in the zap block" do + it "does not report an offense" do + expect_no_offenses(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + zap trash: "~/Library/Application Support/FooBar" + end + CASK + expect_no_offenses(<<~CASK) + cask "foo" do + url "https://example.com/foo.zip" + + zap trash: [ + "~/Library/Application Support/FooBar", + "~/Library/Application Support/FooBarBar", + ] + end + CASK + end + end +end