From 15605e184864598ae69861f9248c5b385e194b35 Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Wed, 1 Jan 2025 03:08:55 +0000 Subject: [PATCH] Update `Rails/IndexBy` and `Rails/IndexWith` to support numbered block parameters --- changelog/fix_index_by_index_with_numblock.md | 1 + lib/rubocop/cop/mixin/index_method.rb | 6 +- lib/rubocop/cop/rails/index_by.rb | 40 +++++++++---- lib/rubocop/cop/rails/index_with.rb | 40 +++++++++---- spec/rubocop/cop/rails/index_by_spec.rb | 59 +++++++++++++++++++ spec/rubocop/cop/rails/index_with_spec.rb | 59 +++++++++++++++++++ 6 files changed, 180 insertions(+), 25 deletions(-) create mode 100644 changelog/fix_index_by_index_with_numblock.md diff --git a/changelog/fix_index_by_index_with_numblock.md b/changelog/fix_index_by_index_with_numblock.md new file mode 100644 index 0000000000..cdbccd7054 --- /dev/null +++ b/changelog/fix_index_by_index_with_numblock.md @@ -0,0 +1 @@ +* [#1404](https://github.com/rubocop/rubocop-rails/pull/1404): Update `Rails/IndexBy` and `Rails/IndexWith` to support numbered block parameters. ([@eugeneius][]) diff --git a/lib/rubocop/cop/mixin/index_method.rb b/lib/rubocop/cop/mixin/index_method.rb index 529639c467..9174cebb35 100644 --- a/lib/rubocop/cop/mixin/index_method.rb +++ b/lib/rubocop/cop/mixin/index_method.rb @@ -6,7 +6,7 @@ module Cop module IndexMethod # rubocop:disable Metrics/ModuleLength RESTRICT_ON_SEND = %i[each_with_object to_h map collect []].freeze - def on_block(node) # rubocop:todo InternalAffairs/NumblockHandler + def on_block(node) on_bad_each_with_object(node) do |*match| handle_possible_offense(node, match, 'each_with_object') end @@ -18,6 +18,8 @@ def on_block(node) # rubocop:todo InternalAffairs/NumblockHandler end end + alias on_numblock on_block + def on_send(node) on_bad_map_to_h(node) do |*match| handle_possible_offense(node, match, 'map { ... }.to_h') @@ -153,6 +155,8 @@ def set_new_method_name(new_method_name, corrector) end def set_new_arg_name(transformed_argname, corrector) + return if block_node.numblock_type? + corrector.replace(block_node.arguments, "|#{transformed_argname}|") end diff --git a/lib/rubocop/cop/rails/index_by.rb b/lib/rubocop/cop/rails/index_by.rb index 646cf8ebda..e89b24bb4a 100644 --- a/lib/rubocop/cop/rails/index_by.rb +++ b/lib/rubocop/cop/rails/index_by.rb @@ -29,18 +29,28 @@ class IndexBy < Base PATTERN def_node_matcher :on_bad_to_h, <<~PATTERN - (block - (call _ :to_h) - (args (arg $_el)) - (array $_ (lvar _el))) + { + (block + (call _ :to_h) + (args (arg $_el)) + (array $_ (lvar _el))) + (numblock + (call _ :to_h) $1 + (array $_ (lvar :_1))) + } PATTERN def_node_matcher :on_bad_map_to_h, <<~PATTERN (call - (block - (call _ {:map :collect}) - (args (arg $_el)) - (array $_ (lvar _el))) + { + (block + (call _ {:map :collect}) + (args (arg $_el)) + (array $_ (lvar _el))) + (numblock + (call _ {:map :collect}) $1 + (array $_ (lvar :_1))) + } :to_h) PATTERN @@ -48,10 +58,16 @@ class IndexBy < Base (send (const {nil? cbase} :Hash) :[] - (block - (call _ {:map :collect}) - (args (arg $_el)) - (array $_ (lvar _el)))) + { + (block + (call _ {:map :collect}) + (args (arg $_el)) + (array $_ (lvar _el))) + (numblock + (call _ {:map :collect}) $1 + (array $_ (lvar :_1))) + } + ) PATTERN private diff --git a/lib/rubocop/cop/rails/index_with.rb b/lib/rubocop/cop/rails/index_with.rb index 90cc3be458..fc72c51646 100644 --- a/lib/rubocop/cop/rails/index_with.rb +++ b/lib/rubocop/cop/rails/index_with.rb @@ -32,18 +32,28 @@ class IndexWith < Base PATTERN def_node_matcher :on_bad_to_h, <<~PATTERN - (block - (call _ :to_h) - (args (arg $_el)) - (array (lvar _el) $_)) + { + (block + (call _ :to_h) + (args (arg $_el)) + (array (lvar _el) $_)) + (numblock + (call _ :to_h) $1 + (array (lvar :_1) $_)) + } PATTERN def_node_matcher :on_bad_map_to_h, <<~PATTERN (call - (block - (call _ {:map :collect}) - (args (arg $_el)) - (array (lvar _el) $_)) + { + (block + (call _ {:map :collect}) + (args (arg $_el)) + (array (lvar _el) $_)) + (numblock + (call _ {:map :collect}) $1 + (array (lvar :_1) $_)) + } :to_h) PATTERN @@ -51,10 +61,16 @@ class IndexWith < Base (send (const {nil? cbase} :Hash) :[] - (block - (call _ {:map :collect}) - (args (arg $_el)) - (array (lvar _el) $_))) + { + (block + (call _ {:map :collect}) + (args (arg $_el)) + (array (lvar _el) $_)) + (numblock + (call _ {:map :collect}) $1 + (array (lvar :_1) $_)) + } + ) PATTERN private diff --git a/spec/rubocop/cop/rails/index_by_spec.rb b/spec/rubocop/cop/rails/index_by_spec.rb index 01afddca49..5d7f4bf6d8 100644 --- a/spec/rubocop/cop/rails/index_by_spec.rb +++ b/spec/rubocop/cop/rails/index_by_spec.rb @@ -182,4 +182,63 @@ RUBY end end + + context 'numbered parameters' do + it 'registers an offense for `map { ... }.to_h`' do + expect_offense(<<~RUBY) + x.map { [_1.to_sym, _1] }.to_h + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `map { ... }.to_h`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { _1.to_sym } + RUBY + end + + context 'when values are transformed' do + it 'does not register an offense for `map { ... }.to_h`' do + expect_no_offenses(<<~RUBY) + x.map { [_1.to_sym, foo(_1)] }.to_h + RUBY + end + end + + it 'registers an offense for Hash[map { ... }]' do + expect_offense(<<~RUBY) + Hash[x.map { [_1.to_sym, _1] }] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `Hash[map { ... }]`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { _1.to_sym } + RUBY + end + + context 'when the referenced numbered parameter is not _1' do + it 'does not register an offense for Hash[map { ... }]' do + expect_no_offenses(<<~RUBY) + Hash[x.map { [_1.to_sym, _2] }] + RUBY + end + end + + it 'registers an offense for `to_h { ... }`' do + expect_offense(<<~RUBY) + x.to_h { [_1.to_sym, _1] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_by` over `to_h { ... }`. + RUBY + + expect_correction(<<~RUBY) + x.index_by { _1.to_sym } + RUBY + end + + context 'when a numbered parameter other than _1 is referenced in the key' do + it 'does not register an offense for `to_h { ... }`' do + expect_no_offenses(<<~RUBY) + x.to_h { [_2.to_sym, _1] } + RUBY + end + end + end end diff --git a/spec/rubocop/cop/rails/index_with_spec.rb b/spec/rubocop/cop/rails/index_with_spec.rb index 32fb4e06ff..ea49367af7 100644 --- a/spec/rubocop/cop/rails/index_with_spec.rb +++ b/spec/rubocop/cop/rails/index_with_spec.rb @@ -155,6 +155,65 @@ RUBY end end + + context 'numbered parameters' do + it 'registers an offense for `map { ... }.to_h`' do + expect_offense(<<~RUBY) + x.map { [_1, _1.to_sym] }.to_h + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `map { ... }.to_h`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { _1.to_sym } + RUBY + end + + context 'when keys are transformed' do + it 'does not register an offense for `map { ... }.to_h`' do + expect_no_offenses(<<~RUBY) + x.map { [foo(_1), _1.to_sym] }.to_h + RUBY + end + end + + it 'registers an offense for Hash[map { ... }]' do + expect_offense(<<~RUBY) + Hash[x.map { [_1, _1.to_sym] }] + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `Hash[map { ... }]`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { _1.to_sym } + RUBY + end + + context 'when the referenced numbered parameter is not _1' do + it 'does not register an offense for Hash[map { ... }]' do + expect_no_offenses(<<~RUBY) + Hash[x.map { [_2, _1.to_sym] }] + RUBY + end + end + + it 'registers an offense for `to_h { ... }`' do + expect_offense(<<~RUBY) + x.to_h { [_1, _1.to_sym] } + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `index_with` over `to_h { ... }`. + RUBY + + expect_correction(<<~RUBY) + x.index_with { _1.to_sym } + RUBY + end + + context 'when a numbered parameter other than _1 is referenced in the value' do + it 'does not register an offense for `to_h { ... }`' do + expect_no_offenses(<<~RUBY) + x.to_h { [_1, _2.to_sym] } + RUBY + end + end + end end context 'when using Rails 5.2 or older', :rails52 do