diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 6ca13bce31..77c0b1a0db 100644 --- a/Library/.rubocop.yml +++ b/Library/.rubocop.yml @@ -56,6 +56,10 @@ Homebrew/Blank: # Core extensions are not available here: - "Homebrew/rubocops/**/*" - "Homebrew/startup/bootsnap.rb" +Homebrew/CompactBlank: + Exclude: + # `blank?` is not necessarily available here: + - "Homebrew/extend/enumerable.rb" # only used internally Homebrew/MoveToExtendOS: @@ -216,8 +220,6 @@ Rails: - "Homebrew/rubocops/**/*" # These relate to ActiveSupport and not other parts of Rails. -Rails/CompactBlank: - Enabled: true Rails/Presence: Enabled: true Rails/SafeNavigationWithBlank: diff --git a/Library/Homebrew/cask/artifact/abstract_artifact.rb b/Library/Homebrew/cask/artifact/abstract_artifact.rb index dcfddaca46..e13d9cc542 100644 --- a/Library/Homebrew/cask/artifact/abstract_artifact.rb +++ b/Library/Homebrew/cask/artifact/abstract_artifact.rb @@ -155,7 +155,7 @@ module Cask end def to_args - @dsl_args.reject(&:blank?) + @dsl_args.compact_blank end end end diff --git a/Library/Homebrew/github_packages.rb b/Library/Homebrew/github_packages.rb index 1bcd1ccb59..75c8b62604 100644 --- a/Library/Homebrew/github_packages.rb +++ b/Library/Homebrew/github_packages.rb @@ -314,7 +314,7 @@ class GitHubPackages "org.opencontainers.image.url" => bottle_hash["formula"]["homepage"], "org.opencontainers.image.vendor" => org, "org.opencontainers.image.version" => version, - }.reject { |_, v| v.blank? } + }.compact_blank manifests = [] end @@ -369,7 +369,7 @@ class GitHubPackages architecture: architecture, os: os, "os.version" => os_version, - }.reject { |_, v| v.blank? } + }.compact_blank tar_sha256 = Digest::SHA256.new Zlib::GzipReader.open(local_file) do |gz| @@ -391,7 +391,7 @@ class GitHubPackages "sh.brew.bottle.glibc.version" => glibc_version, "sh.brew.bottle.size" => local_file_size.to_s, "sh.brew.tab" => tab.to_json, - }.reject { |_, v| v.blank? } + }.compact_blank annotations_hash = formula_annotations_hash.merge(descriptor_annotations_hash).merge( { @@ -399,7 +399,7 @@ class GitHubPackages "org.opencontainers.image.documentation" => documentation, "org.opencontainers.image.title" => "#{formula_full_name} #{tag}", }, - ).reject { |_, v| v.blank? }.sort.to_h + ).compact_blank.sort.to_h image_manifest = { schemaVersion: 2, diff --git a/Library/Homebrew/livecheck/strategy/github_releases.rb b/Library/Homebrew/livecheck/strategy/github_releases.rb index 7bd04c024b..1d4300cf15 100644 --- a/Library/Homebrew/livecheck/strategy/github_releases.rb +++ b/Library/Homebrew/livecheck/strategy/github_releases.rb @@ -105,7 +105,7 @@ module Homebrew end content = [content] unless content.is_a?(Array) - content.reject(&:blank?).map do |release| + content.compact_blank.map do |release| next if release["draft"] || release["prerelease"] value = T.let(nil, T.untyped) diff --git a/Library/Homebrew/rubocops/all.rb b/Library/Homebrew/rubocops/all.rb index 4fee6b2e24..be71a3aca3 100644 --- a/Library/Homebrew/rubocops/all.rb +++ b/Library/Homebrew/rubocops/all.rb @@ -3,6 +3,7 @@ require_relative "../extend/array" require_relative "blank" +require_relative "compact_blank" require_relative "io_read" require_relative "move_to_extend_os" require_relative "present" diff --git a/Library/Homebrew/rubocops/compact_blank.rb b/Library/Homebrew/rubocops/compact_blank.rb new file mode 100644 index 0000000000..6539878f1f --- /dev/null +++ b/Library/Homebrew/rubocops/compact_blank.rb @@ -0,0 +1,109 @@ +# typed: true +# frozen_string_literal: true + +module RuboCop + module Cop + module Homebrew + # Checks if collection can be blank-compacted with `compact_blank`. + # + # @note + # It is unsafe by default because false positives may occur in the + # blank check of block arguments to the receiver object. + # + # For example, `[[1, 2], [3, nil]].reject { |first, second| second.blank? }` and + # `[[1, 2], [3, nil]].compact_blank` are not compatible. The same is true for `blank?`. + # This will work fine when the receiver is a hash object. + # + # And `compact_blank!` has different implementations for `Array`, `Hash`, and + # `ActionController::Parameters`. + # `Array#compact_blank!`, `Hash#compact_blank!` are equivalent to `delete_if(&:blank?)`. + # `ActionController::Parameters#compact_blank!` is equivalent to `reject!(&:blank?)`. + # If the cop makes a mistake, autocorrected code may get unexpected behavior. + # + # @example + # + # # bad + # collection.reject(&:blank?) + # collection.reject { |_k, v| v.blank? } + # + # # good + # collection.compact_blank + # + # # bad + # collection.delete_if(&:blank?) # Same behavior as `Array#compact_blank!` and `Hash#compact_blank!` + # collection.delete_if { |_, v| v.blank? } # Same behavior as `Array#compact_blank!` and `Hash#compact_blank!` + # collection.reject!(&:blank?) # Same behavior as `ActionController::Parameters#compact_blank!` + # collection.reject! { |_k, v| v.blank? } # Same behavior as `ActionController::Parameters#compact_blank!` + # + # # good + # collection.compact_blank! + # + class CompactBlank < Base + include RangeHelp + extend AutoCorrector + + MSG = "Use `%s` instead." + RESTRICT_ON_SEND = [:reject, :delete_if, :reject!].freeze + + def_node_matcher :reject_with_block?, <<~PATTERN + (block + (send _ {:reject :delete_if :reject!}) + $(args ...) + (send + $(lvar _) :blank?)) + PATTERN + + def_node_matcher :reject_with_block_pass?, <<~PATTERN + (send _ {:reject :delete_if :reject!} + (block_pass + (sym :blank?))) + PATTERN + + def on_send(node) + return unless bad_method?(node) + + range = offense_range(node) + preferred_method = preferred_method(node) + add_offense(range, message: format(MSG, preferred_method: preferred_method)) do |corrector| + corrector.replace(range, preferred_method) + end + end + + private + + def bad_method?(node) + return true if reject_with_block_pass?(node) + + if (arguments, receiver_in_block = reject_with_block?(node.parent)) + return use_single_value_block_argument?(arguments, receiver_in_block) || + use_hash_value_block_argument?(arguments, receiver_in_block) + end + + false + end + + def use_single_value_block_argument?(arguments, receiver_in_block) + arguments.length == 1 && arguments[0].source == receiver_in_block.source + end + + def use_hash_value_block_argument?(arguments, receiver_in_block) + arguments.length == 2 && arguments[1].source == receiver_in_block.source + end + + def offense_range(node) + end_pos = if node.parent&.block_type? && node.parent&.send_node == node + node.parent.source_range.end_pos + else + node.source_range.end_pos + end + + range_between(node.loc.selector.begin_pos, end_pos) + end + + def preferred_method(node) + node.method?(:reject) ? "compact_blank" : "compact_blank!" + end + end + end + end +end diff --git a/Library/Homebrew/test/rubocops/compact_blank_spec.rb b/Library/Homebrew/test/rubocops/compact_blank_spec.rb new file mode 100644 index 0000000000..5762e63f57 --- /dev/null +++ b/Library/Homebrew/test/rubocops/compact_blank_spec.rb @@ -0,0 +1,116 @@ +# frozen_string_literal: true + +require "rubocops/compact_blank" + +RSpec.describe RuboCop::Cop::Homebrew::CompactBlank do + subject(:cop) { described_class.new } + + it "registers and corrects an offense when using `reject { |e| e.blank? }`" do + expect_offense(<<~RUBY) + collection.reject { |e| e.blank? } + ^^^^^^^^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank` instead. + RUBY + + expect_correction(<<~RUBY) + collection.compact_blank + RUBY + end + + it "registers and corrects an offense when using `reject(&:blank?)`" do + expect_offense(<<~RUBY) + collection.reject(&:blank?) + ^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank` instead. + RUBY + + expect_correction(<<~RUBY) + collection.compact_blank + RUBY + end + + it "registers and corrects an offense when using `delete_if { |e| e.blank? }`" do + expect_offense(<<~RUBY) + collection.delete_if { |e| e.blank? } + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank!` instead. + RUBY + + expect_correction(<<~RUBY) + collection.compact_blank! + RUBY + end + + it "registers and corrects an offense when using `delete_if(&:blank?)`" do + expect_offense(<<~RUBY) + collection.delete_if(&:blank?) + ^^^^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank!` instead. + RUBY + + expect_correction(<<~RUBY) + collection.compact_blank! + RUBY + end + + it "registers and corrects an offense when using `reject! { |e| e.blank? }`" do + expect_offense(<<~RUBY) + collection.reject! { |e| e.blank? } + ^^^^^^^^^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank!` instead. + RUBY + + expect_correction(<<~RUBY) + collection.compact_blank! + RUBY + end + + it "registers and corrects an offense when using `reject!(&:blank?)`" do + expect_offense(<<~RUBY) + collection.reject!(&:blank?) + ^^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank!` instead. + RUBY + + expect_correction(<<~RUBY) + collection.compact_blank! + RUBY + end + + it "registers and corrects an offense when using `reject(&:blank?)` in block" do + expect_offense(<<~RUBY) + hash.transform_values { |value| value.reject(&:blank?) } + ^^^^^^^^^^^^^^^^ Homebrew/CompactBlank: Use `compact_blank` instead. + RUBY + + expect_correction(<<~RUBY) + hash.transform_values { |value| value.compact_blank } + RUBY + end + + it "does not register an offense when using `compact_blank`" do + expect_no_offenses(<<~RUBY) + collection.compact_blank + RUBY + end + + it "does not register an offense when using `compact_blank!`" do + expect_no_offenses(<<~RUBY) + collection.compact_blank! + RUBY + end + + it "does not register an offense when using `reject { |k, v| k.blank? }`" do + expect_no_offenses(<<~RUBY) + collection.reject { |k, v| k.blank? } + RUBY + end + + it "does not register an offense when using the receiver of `blank?` is not a block variable" do + expect_no_offenses(<<~RUBY) + def foo(arg) + collection.reject { |_| arg.blank? } + end + RUBY + end + + it "does not register an offense when using `reject { |e| e.empty? }`" do + expect_no_offenses(<<~RUBY) + collection.reject { |e| e.empty? } + RUBY + end +end