From 57b3b09c6f3c77b7f1b7d3f04ae66c53d9bc9bb7 Mon Sep 17 00:00:00 2001 From: Vlad Pisanov Date: Mon, 8 May 2023 22:29:17 -0400 Subject: [PATCH] Add `sort!` and `minmax` to `Performance/CompareWithBlock` --- .../change_compare_with_block_methods.md | 1 + .../cop/performance/compare_with_block.rb | 29 +++++++++++------- .../performance/compare_with_block_spec.rb | 30 ++++++++++--------- 3 files changed, 36 insertions(+), 24 deletions(-) create mode 100644 changelog/change_compare_with_block_methods.md diff --git a/changelog/change_compare_with_block_methods.md b/changelog/change_compare_with_block_methods.md new file mode 100644 index 0000000000..99c6c1556e --- /dev/null +++ b/changelog/change_compare_with_block_methods.md @@ -0,0 +1 @@ +* [#357](https://github.com/rubocop/rubocop-performance/pull/357): Add `sort!` and `minmax` to `Performance/CompareWithBlock`. ([@vlad-pisanov][]) diff --git a/lib/rubocop/cop/performance/compare_with_block.rb b/lib/rubocop/cop/performance/compare_with_block.rb index cfbbb1de9b..9ab6e2cd81 100644 --- a/lib/rubocop/cop/performance/compare_with_block.rb +++ b/lib/rubocop/cop/performance/compare_with_block.rb @@ -5,35 +5,42 @@ module Cop module Performance # Identifies places where `sort { |a, b| a.foo <=> b.foo }` # can be replaced by `sort_by(&:foo)`. - # This cop also checks `max` and `min` methods. + # This cop also checks `sort!`, `min`, `max` and `minmax` methods. # # @example # # bad - # array.sort { |a, b| a.foo <=> b.foo } - # array.max { |a, b| a.foo <=> b.foo } - # array.min { |a, b| a.foo <=> b.foo } - # array.sort { |a, b| a[:foo] <=> b[:foo] } + # array.sort { |a, b| a.foo <=> b.foo } + # array.sort! { |a, b| a.foo <=> b.foo } + # array.max { |a, b| a.foo <=> b.foo } + # array.min { |a, b| a.foo <=> b.foo } + # array.minmax { |a, b| a.foo <=> b.foo } + # array.sort { |a, b| a[:foo] <=> b[:foo] } # # # good # array.sort_by(&:foo) + # array.sort_by!(&:foo) # array.sort_by { |v| v.foo } # array.sort_by do |var| # var.foo # end # array.max_by(&:foo) # array.min_by(&:foo) + # array.minmax_by(&:foo) # array.sort_by { |a| a[:foo] } class CompareWithBlock < Base include RangeHelp extend AutoCorrector - MSG = 'Use `%s_by%s` instead of ' \ + MSG = 'Use `%s%s` instead of ' \ '`%s { |%s, %s| %s ' \ '<=> %s }`.' + REPLACEMENT = { sort: :sort_by, sort!: :sort_by!, min: :min_by, max: :max_by, minmax: :minmax_by }.freeze + private_constant :REPLACEMENT + def_node_matcher :compare?, <<~PATTERN (block - $(send _ {:sort :min :max}) + $(send _ {:sort :sort! :min :max :minmax}) (args (arg $_a) (arg $_b)) $send) PATTERN @@ -54,9 +61,9 @@ def on_block(node) add_offense(range, message: message(send, method, var_a, var_b, args_a)) do |corrector| replacement = if method == :[] - "#{send.method_name}_by { |a| a[#{args_a.first.source}] }" + "#{REPLACEMENT[send.method_name]} { |a| a[#{args_a.first.source}] }" else - "#{send.method_name}_by(&:#{method})" + "#{REPLACEMENT[send.method_name]}(&:#{method})" end corrector.replace(range, replacement) end @@ -82,7 +89,8 @@ def slow_compare?(method, args_a, args_b) # rubocop:disable Metrics/MethodLength def message(send, method, var_a, var_b, args) - compare_method = send.method_name + compare_method = send.method_name + replacement_method = REPLACEMENT[compare_method] if method == :[] key = args.first instead = " { |a| a[#{key.source}] }" @@ -94,6 +102,7 @@ def message(send, method, var_a, var_b, args) str_b = "#{var_b}.#{method}" end format(MSG, compare_method: compare_method, + replacement_method: replacement_method, instead: instead, var_a: var_a, var_b: var_b, diff --git a/spec/rubocop/cop/performance/compare_with_block_spec.rb b/spec/rubocop/cop/performance/compare_with_block_spec.rb index 25de7d52d6..83b6438b17 100644 --- a/spec/rubocop/cop/performance/compare_with_block_spec.rb +++ b/spec/rubocop/cop/performance/compare_with_block_spec.rb @@ -1,48 +1,48 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Performance::CompareWithBlock, :config do - shared_examples 'compare with block' do |method| + shared_examples 'compare with block' do |method, replacement| it "registers an offense and corrects for #{method}" do expect_offense(<<~RUBY, method: method) array.#{method} { |a, b| a.foo <=> b.foo } - ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by(&:foo)` instead of `#{method} { |a, b| a.foo <=> b.foo }`. + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement}(&:foo)` instead of `#{method} { |a, b| a.foo <=> b.foo }`. RUBY expect_correction(<<~RUBY) - array.#{method}_by(&:foo) + array.#{replacement}(&:foo) RUBY end it "registers an offense and corrects for #{method} with [:foo]" do expect_offense(<<~RUBY, method: method) array.#{method} { |a, b| a[:foo] <=> b[:foo] } - ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by { |a| a[:foo] }` instead of `#{method} { |a, b| a[:foo] <=> b[:foo] }`. + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement} { |a| a[:foo] }` instead of `#{method} { |a, b| a[:foo] <=> b[:foo] }`. RUBY expect_correction(<<~RUBY) - array.#{method}_by { |a| a[:foo] } + array.#{replacement} { |a| a[:foo] } RUBY end it "registers an offense and corrects for #{method} with ['foo']" do expect_offense(<<~RUBY, method: method) array.#{method} { |a, b| a['foo'] <=> b['foo'] } - ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by { |a| a['foo'] }` instead of `#{method} { |a, b| a['foo'] <=> b['foo'] }`. + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement} { |a| a['foo'] }` instead of `#{method} { |a, b| a['foo'] <=> b['foo'] }`. RUBY expect_correction(<<~RUBY) - array.#{method}_by { |a| a['foo'] } + array.#{replacement} { |a| a['foo'] } RUBY end it "registers an offense and corrects for #{method} with [1]" do expect_offense(<<~RUBY, method: method) array.#{method} { |a, b| a[1] <=> b[1] } - ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{method}_by { |a| a[1] }` instead of `#{method} { |a, b| a[1] <=> b[1] }`. + ^{method}^^^^^^^^^^^^^^^^^^^^^^^^^ Use `#{replacement} { |a| a[1] }` instead of `#{method} { |a, b| a[1] <=> b[1] }`. RUBY expect_correction(<<~RUBY) - array.#{method}_by { |a| a[1] } + array.#{replacement} { |a| a[1] } RUBY end @@ -50,12 +50,14 @@ expect_no_offenses("array.#{method} { |a, b| b <=> a }") end - it "accepts #{method}_by" do - expect_no_offenses("array.#{method}_by { |a| a.baz }") + it "accepts #{replacement}" do + expect_no_offenses("array.#{replacement} { |a| a.baz }") end end - include_examples 'compare with block', 'sort' - include_examples 'compare with block', 'max' - include_examples 'compare with block', 'min' + include_examples 'compare with block', 'sort', 'sort_by' + include_examples 'compare with block', 'sort!', 'sort_by!' + include_examples 'compare with block', 'max', 'max_by' + include_examples 'compare with block', 'min', 'min_by' + include_examples 'compare with block', 'minmax', 'minmax_by' end