From 7c672a02e1f52b95ab878aeaad3f78de9efaa3bd Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 5 Dec 2023 18:38:52 +0900 Subject: [PATCH] Make some cops aware of safe navigation operator Fixes #1191, #1192, #1193, #1194, #1196, #1197, #1201, and #1202. This PR makes `Rails/ActiveSupportAliases`, `Rails/FindBy`, `Rails/FindById`, `Rails/Inquiry`, `Rails/Pick` `Rails/PluckId`, `Rails/PluckInWhere`, `Rails/WhereEquals`, `Rails/WhereExists`, and `Rails/WhereNot` cops aware of safe navigation operator. --- ..._cops_aware_of_safe_navigation_operator.md | 1 + lib/rubocop/cop/mixin/active_record_helper.rb | 2 +- .../cop/rails/active_support_aliases.rb | 11 ++-- lib/rubocop/cop/rails/find_by.rb | 4 +- lib/rubocop/cop/rails/find_by_id.rb | 32 +++------- lib/rubocop/cop/rails/inquiry.rb | 1 + lib/rubocop/cop/rails/pick.rb | 11 ++-- lib/rubocop/cop/rails/pluck_id.rb | 3 +- lib/rubocop/cop/rails/pluck_in_where.rb | 3 +- lib/rubocop/cop/rails/where_equals.rb | 5 +- lib/rubocop/cop/rails/where_exists.rb | 17 +++--- lib/rubocop/cop/rails/where_not.rb | 13 +++-- .../cop/rails/active_support_aliases_spec.rb | 58 +++++++++++++++++-- spec/rubocop/cop/rails/find_by_id_spec.rb | 44 ++++++++++++++ spec/rubocop/cop/rails/find_by_spec.rb | 22 +++++++ spec/rubocop/cop/rails/inquiry_spec.rb | 7 +++ spec/rubocop/cop/rails/pick_spec.rb | 30 ++++++++-- spec/rubocop/cop/rails/pluck_id_spec.rb | 11 ++++ spec/rubocop/cop/rails/pluck_in_where_spec.rb | 11 ++++ spec/rubocop/cop/rails/where_equals_spec.rb | 22 +++++++ spec/rubocop/cop/rails/where_exists_spec.rb | 33 +++++++++++ spec/rubocop/cop/rails/where_not_spec.rb | 22 +++++++ 22 files changed, 301 insertions(+), 62 deletions(-) create mode 100644 changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md diff --git a/changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md b/changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md new file mode 100644 index 0000000000..31fffba920 --- /dev/null +++ b/changelog/fix_make_some_cops_aware_of_safe_navigation_operator.md @@ -0,0 +1 @@ +* [#1204](https://github.com/rubocop/rubocop-rails/pull/1204): Make `Rails/ActiveSupportAliases`, `Rails/FindBy`, `Rails/FindById`, `Rails/Inquiry`, `Rails/Pick` `Rails/PluckId`, `Rails/PluckInWhere`, `Rails/WhereEquals`, `Rails/WhereExists`, and `Rails/WhereNot` cops aware of safe navigation operator. ([@koic][]) diff --git a/lib/rubocop/cop/mixin/active_record_helper.rb b/lib/rubocop/cop/mixin/active_record_helper.rb index 0bbe4e954c..e5b5b5903c 100644 --- a/lib/rubocop/cop/mixin/active_record_helper.rb +++ b/lib/rubocop/cop/mixin/active_record_helper.rb @@ -98,7 +98,7 @@ def polymorphic?(belongs_to) end def in_where?(node) - send_node = node.each_ancestor(:send).first + send_node = node.each_ancestor(:send, :csend).first return false unless send_node return true if WHERE_METHODS.include?(send_node.method_name) diff --git a/lib/rubocop/cop/rails/active_support_aliases.rb b/lib/rubocop/cop/rails/active_support_aliases.rb index 312490da4b..50bf431ce4 100644 --- a/lib/rubocop/cop/rails/active_support_aliases.rb +++ b/lib/rubocop/cop/rails/active_support_aliases.rb @@ -27,13 +27,13 @@ class ActiveSupportAliases < Base ALIASES = { starts_with?: { - original: :start_with?, matcher: '(send str :starts_with? _)' + original: :start_with?, matcher: '(call str :starts_with? _)' }, ends_with?: { - original: :end_with?, matcher: '(send str :ends_with? _)' + original: :end_with?, matcher: '(call str :ends_with? _)' }, - append: { original: :<<, matcher: '(send array :append _)' }, - prepend: { original: :unshift, matcher: '(send array :prepend _)' } + append: { original: :<<, matcher: '(call array :append _)' }, + prepend: { original: :unshift, matcher: '(call array :prepend _)' } }.freeze ALIASES.each do |aliased_method, options| @@ -47,13 +47,14 @@ def on_send(node) preferred_method = ALIASES[aliased_method][:original] message = format(MSG, prefer: preferred_method, current: aliased_method) - add_offense(node, message: message) do |corrector| + add_offense(node.loc.selector.join(node.source_range.end), message: message) do |corrector| next if append(node) corrector.replace(node.loc.selector, preferred_method) end end end + alias on_csend on_send end end end diff --git a/lib/rubocop/cop/rails/find_by.rb b/lib/rubocop/cop/rails/find_by.rb index 74da0840f3..28ad3200ef 100644 --- a/lib/rubocop/cop/rails/find_by.rb +++ b/lib/rubocop/cop/rails/find_by.rb @@ -28,7 +28,7 @@ class FindBy < Base include RangeHelp extend AutoCorrector - MSG = 'Use `find_by` instead of `where.%s`.' + MSG = 'Use `find_by` instead of `where%s%s`.' RESTRICT_ON_SEND = %i[first take].freeze def on_send(node) @@ -37,7 +37,7 @@ def on_send(node) range = offense_range(node) - add_offense(range, message: format(MSG, method: node.method_name)) do |corrector| + add_offense(range, message: format(MSG, dot: node.loc.dot.source, method: node.method_name)) do |corrector| autocorrect(corrector, node) end end diff --git a/lib/rubocop/cop/rails/find_by_id.rb b/lib/rubocop/cop/rails/find_by_id.rb index 9e6dc1c274..10bd10b2b4 100644 --- a/lib/rubocop/cop/rails/find_by_id.rb +++ b/lib/rubocop/cop/rails/find_by_id.rb @@ -24,40 +24,39 @@ class FindById < Base RESTRICT_ON_SEND = %i[take! find_by_id! find_by!].freeze def_node_matcher :where_take?, <<~PATTERN - (send - $(send _ :where + (call + $(call _ :where (hash (pair (sym :id) $_))) :take!) PATTERN def_node_matcher :find_by?, <<~PATTERN { - (send _ :find_by_id! $_) - (send _ :find_by! (hash (pair (sym :id) $_))) + (call _ :find_by_id! $_) + (call _ :find_by! (hash (pair (sym :id) $_))) } PATTERN def on_send(node) where_take?(node) do |where, id_value| range = where_take_offense_range(node, where) - bad_method = build_where_take_bad_method(id_value) - register_offense(range, id_value, bad_method) + register_offense(range, id_value) end find_by?(node) do |id_value| range = find_by_offense_range(node) - bad_method = build_find_by_bad_method(node, id_value) - register_offense(range, id_value, bad_method) + register_offense(range, id_value) end end + alias on_csend on_send private - def register_offense(range, id_value, bad_method) + def register_offense(range, id_value) good_method = build_good_method(id_value) - message = format(MSG, good_method: good_method, bad_method: bad_method) + message = format(MSG, good_method: good_method, bad_method: range.source) add_offense(range, message: message) do |corrector| corrector.replace(range, good_method) @@ -75,19 +74,6 @@ def find_by_offense_range(node) def build_good_method(id_value) "find(#{id_value.source})" end - - def build_where_take_bad_method(id_value) - "where(id: #{id_value.source}).take!" - end - - def build_find_by_bad_method(node, id_value) - case node.method_name - when :find_by_id! - "find_by_id!(#{id_value.source})" - when :find_by! - "find_by!(id: #{id_value.source})" - end - end end end end diff --git a/lib/rubocop/cop/rails/inquiry.rb b/lib/rubocop/cop/rails/inquiry.rb index a7933a9347..45ad7ecd65 100644 --- a/lib/rubocop/cop/rails/inquiry.rb +++ b/lib/rubocop/cop/rails/inquiry.rb @@ -33,6 +33,7 @@ def on_send(node) add_offense(node.loc.selector) end + alias on_csend on_send end end end diff --git a/lib/rubocop/cop/rails/pick.rb b/lib/rubocop/cop/rails/pick.rb index a13ba846f0..f4f344ff9c 100644 --- a/lib/rubocop/cop/rails/pick.rb +++ b/lib/rubocop/cop/rails/pick.rb @@ -28,13 +28,13 @@ class Pick < Base extend AutoCorrector extend TargetRailsVersion - MSG = 'Prefer `pick(%s)` over `pluck(%s).first`.' + MSG = 'Prefer `pick(%s)` over `%s`.' RESTRICT_ON_SEND = %i[first].freeze minimum_target_rails_version 6.0 def_node_matcher :pick_candidate?, <<~PATTERN - (send (send _ :pluck ...) :first) + (call (call _ :pluck ...) :first) PATTERN def on_send(node) @@ -44,7 +44,7 @@ def on_send(node) node_selector = node.loc.selector range = receiver_selector.join(node_selector) - add_offense(range, message: message(receiver)) do |corrector| + add_offense(range, message: message(receiver, range)) do |corrector| first_range = receiver.source_range.end.join(node_selector) corrector.remove(first_range) @@ -52,11 +52,12 @@ def on_send(node) end end end + alias on_csend on_send private - def message(receiver) - format(MSG, args: receiver.arguments.map(&:source).join(', ')) + def message(receiver, current) + format(MSG, args: receiver.arguments.map(&:source).join(', '), current: current.source) end end end diff --git a/lib/rubocop/cop/rails/pluck_id.rb b/lib/rubocop/cop/rails/pluck_id.rb index 4ba2e109b2..8ef983c280 100644 --- a/lib/rubocop/cop/rails/pluck_id.rb +++ b/lib/rubocop/cop/rails/pluck_id.rb @@ -34,7 +34,7 @@ class PluckId < Base RESTRICT_ON_SEND = %i[pluck].freeze def_node_matcher :pluck_id_call?, <<~PATTERN - (send _ :pluck {(sym :id) (send nil? :primary_key)}) + (call _ :pluck {(sym :id) (send nil? :primary_key)}) PATTERN def on_send(node) @@ -47,6 +47,7 @@ def on_send(node) corrector.replace(offense_range(node), 'ids') end end + alias on_csend on_send private diff --git a/lib/rubocop/cop/rails/pluck_in_where.rb b/lib/rubocop/cop/rails/pluck_in_where.rb index 7aa17edede..05d0fb9715 100644 --- a/lib/rubocop/cop/rails/pluck_in_where.rb +++ b/lib/rubocop/cop/rails/pluck_in_where.rb @@ -65,13 +65,14 @@ def on_send(node) corrector.replace(range, replacement) end end + alias on_csend on_send private def root_receiver(node) receiver = node.receiver - if receiver&.send_type? + if receiver&.call_type? root_receiver(receiver) else receiver diff --git a/lib/rubocop/cop/rails/where_equals.rb b/lib/rubocop/cop/rails/where_equals.rb index db9072d867..d7a568e8e4 100644 --- a/lib/rubocop/cop/rails/where_equals.rb +++ b/lib/rubocop/cop/rails/where_equals.rb @@ -33,8 +33,8 @@ class WhereEquals < Base def_node_matcher :where_method_call?, <<~PATTERN { - (send _ :where (array $str_type? $_ ?)) - (send _ :where $str_type? $_ ?) + (call _ :where (array $str_type? $_ ?)) + (call _ :where $str_type? $_ ?) } PATTERN @@ -55,6 +55,7 @@ def on_send(node) end end end + alias on_csend on_send EQ_ANONYMOUS_RE = /\A([\w.]+)\s+=\s+\?\z/.freeze # column = ? IN_ANONYMOUS_RE = /\A([\w.]+)\s+IN\s+\(\?\)\z/i.freeze # column IN (?) diff --git a/lib/rubocop/cop/rails/where_exists.rb b/lib/rubocop/cop/rails/where_exists.rb index 8893213416..ff3b064b0c 100644 --- a/lib/rubocop/cop/rails/where_exists.rb +++ b/lib/rubocop/cop/rails/where_exists.rb @@ -55,11 +55,11 @@ class WhereExists < Base RESTRICT_ON_SEND = %i[exists?].freeze def_node_matcher :where_exists_call?, <<~PATTERN - (send (send _ :where $...) :exists?) + (call (call _ :where $...) :exists?) PATTERN def_node_matcher :exists_with_args?, <<~PATTERN - (send _ :exists? $...) + (call _ :exists? $...) PATTERN def on_send(node) @@ -67,7 +67,7 @@ def on_send(node) return unless convertable_args?(args) range = correction_range(node) - good_method = build_good_method(args) + good_method = build_good_method(args, dot_source: node.loc.dot.source) message = format(MSG, good_method: good_method, bad_method: range.source) add_offense(range, message: message) do |corrector| @@ -75,6 +75,7 @@ def on_send(node) end end end + alias on_csend on_send private @@ -108,11 +109,11 @@ def correction_range(node) end end - def build_good_method(args) + def build_good_method(args, dot_source: '.') if exists_style? build_good_method_exists(args) elsif where_style? - build_good_method_where(args) + build_good_method_where(args, dot_source) end end @@ -124,11 +125,11 @@ def build_good_method_exists(args) end end - def build_good_method_where(args) + def build_good_method_where(args, dot_source) if args.size > 1 - "where(#{args.map(&:source).join(', ')}).exists?" + "where(#{args.map(&:source).join(', ')})#{dot_source}exists?" else - "where(#{args[0].source}).exists?" + "where(#{args[0].source})#{dot_source}exists?" end end end diff --git a/lib/rubocop/cop/rails/where_not.rb b/lib/rubocop/cop/rails/where_not.rb index 1e2aab2b65..fd6ad6d65e 100644 --- a/lib/rubocop/cop/rails/where_not.rb +++ b/lib/rubocop/cop/rails/where_not.rb @@ -32,8 +32,8 @@ class WhereNot < Base def_node_matcher :where_method_call?, <<~PATTERN { - (send _ :where (array $str_type? $_ ?)) - (send _ :where $str_type? $_ ?) + (call _ :where (array $str_type? $_ ?)) + (call _ :where $str_type? $_ ?) } PATTERN @@ -46,7 +46,7 @@ def on_send(node) column_and_value = extract_column_and_value(template_node, value_node) return unless column_and_value - good_method = build_good_method(*column_and_value) + good_method = build_good_method(node.loc.dot.source, *column_and_value) message = format(MSG, good_method: good_method) add_offense(range, message: message) do |corrector| @@ -54,6 +54,7 @@ def on_send(node) end end end + alias on_csend on_send NOT_EQ_ANONYMOUS_RE = /\A([\w.]+)\s+(?:!=|<>)\s+\?\z/.freeze # column != ?, column <> ? NOT_IN_ANONYMOUS_RE = /\A([\w.]+)\s+NOT\s+IN\s+\(\?\)\z/i.freeze # column NOT IN (?) @@ -86,13 +87,13 @@ def extract_column_and_value(template_node, value_node) [Regexp.last_match(1), value] end - def build_good_method(column, value) + def build_good_method(dot, column, value) if column.include?('.') table, column = column.split('.') - "where.not(#{table}: { #{column}: #{value} })" + "where#{dot}not(#{table}: { #{column}: #{value} })" else - "where.not(#{column}: #{value})" + "where#{dot}not(#{column}: #{value})" end end end diff --git a/spec/rubocop/cop/rails/active_support_aliases_spec.rb b/spec/rubocop/cop/rails/active_support_aliases_spec.rb index 6aec9ed046..1644e0808b 100644 --- a/spec/rubocop/cop/rails/active_support_aliases_spec.rb +++ b/spec/rubocop/cop/rails/active_support_aliases_spec.rb @@ -6,7 +6,7 @@ it 'registers as an offense and corrects' do expect_offense(<<~RUBY) 'some_string'.starts_with?('prefix') - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `start_with?` instead of `starts_with?`. + ^^^^^^^^^^^^^^^^^^^^^^ Use `start_with?` instead of `starts_with?`. RUBY expect_correction(<<~RUBY) @@ -15,6 +15,19 @@ end end + describe '&.starts_with?' do + it 'registers as an offense and corrects' do + expect_offense(<<~RUBY) + 'some_string'&.starts_with?('prefix') + ^^^^^^^^^^^^^^^^^^^^^^ Use `start_with?` instead of `starts_with?`. + RUBY + + expect_correction(<<~RUBY) + 'some_string'&.start_with?('prefix') + RUBY + end + end + describe '#start_with?' do it 'is not registered as an offense' do expect_no_offenses("'some_string'.start_with?('prefix')") @@ -25,7 +38,7 @@ it 'registers as an offense and corrects' do expect_offense(<<~RUBY) 'some_string'.ends_with?('prefix') - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `end_with?` instead of `ends_with?`. + ^^^^^^^^^^^^^^^^^^^^ Use `end_with?` instead of `ends_with?`. RUBY expect_correction(<<~RUBY) @@ -34,6 +47,19 @@ end end + describe '&ends_with?' do + it 'registers as an offense and corrects' do + expect_offense(<<~RUBY) + 'some_string'&.ends_with?('prefix') + ^^^^^^^^^^^^^^^^^^^^ Use `end_with?` instead of `ends_with?`. + RUBY + + expect_correction(<<~RUBY) + 'some_string'&.end_with?('prefix') + RUBY + end + end + describe '#end_with?' do it 'is not registered as an offense' do expect_no_offenses("'some_string'.end_with?('prefix')") @@ -46,7 +72,18 @@ it 'registers as an offense and does not correct' do expect_offense(<<~RUBY) [1, 'a', 3].append('element') - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `<<` instead of `append`. + ^^^^^^^^^^^^^^^^^ Use `<<` instead of `append`. + RUBY + + expect_no_corrections + end + end + + describe '&.append' do + it 'registers as an offense and does not correct' do + expect_offense(<<~RUBY) + [1, 'a', 3]&.append('element') + ^^^^^^^^^^^^^^^^^ Use `<<` instead of `append`. RUBY expect_no_corrections @@ -63,7 +100,7 @@ it 'registers as an offense and corrects' do expect_offense(<<~RUBY) [1, 'a', 3].prepend('element') - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `unshift` instead of `prepend`. + ^^^^^^^^^^^^^^^^^^ Use `unshift` instead of `prepend`. RUBY expect_correction(<<~RUBY) @@ -72,6 +109,19 @@ end end + describe '&.prepend' do + it 'registers as an offense and corrects' do + expect_offense(<<~RUBY) + [1, 'a', 3]&.prepend('element') + ^^^^^^^^^^^^^^^^^^ Use `unshift` instead of `prepend`. + RUBY + + expect_correction(<<~RUBY) + [1, 'a', 3]&.unshift('element') + RUBY + end + end + describe '#unshift' do it 'is not registered as an offense' do expect_no_offenses("[1, 'a', 3].unshift('element')") diff --git a/spec/rubocop/cop/rails/find_by_id_spec.rb b/spec/rubocop/cop/rails/find_by_id_spec.rb index c04baaef7f..6e3212649b 100644 --- a/spec/rubocop/cop/rails/find_by_id_spec.rb +++ b/spec/rubocop/cop/rails/find_by_id_spec.rb @@ -12,6 +12,28 @@ RUBY end + it 'registers an offense and corrects when using `where(id: ...)&.take!`' do + expect_offense(<<~RUBY) + User.where(id: 1)&.take! + ^^^^^^^^^^^^^^^^^^^ Use `find(1)` instead of `where(id: 1)&.take!`. + RUBY + + expect_correction(<<~RUBY) + User.find(1) + RUBY + end + + it 'registers an offense and corrects when using `where(id: ...)&.take!` with safe navigation' do + expect_offense(<<~RUBY) + User&.where(id: 1)&.take! + ^^^^^^^^^^^^^^^^^^^ Use `find(1)` instead of `where(id: 1)&.take!`. + RUBY + + expect_correction(<<~RUBY) + User&.find(1) + RUBY + end + it 'registers an offense and corrects when using `find_by_id!`' do expect_offense(<<~RUBY) User.find_by_id!(1) @@ -23,6 +45,17 @@ RUBY end + it 'registers an offense and corrects when using `find_by_id!` with safe navigation' do + expect_offense(<<~RUBY) + User&.find_by_id!(1) + ^^^^^^^^^^^^^^ Use `find(1)` instead of `find_by_id!(1)`. + RUBY + + expect_correction(<<~RUBY) + User&.find(1) + RUBY + end + it 'registers an offense and corrects when using `find_by!(id: ...)`' do expect_offense(<<~RUBY) User.find_by!(id: 1) @@ -34,6 +67,17 @@ RUBY end + it 'registers an offense and corrects when using `find_by!(id: ...)` with safe navigation' do + expect_offense(<<~RUBY) + User&.find_by!(id: 1) + ^^^^^^^^^^^^^^^ Use `find(1)` instead of `find_by!(id: 1)`. + RUBY + + expect_correction(<<~RUBY) + User&.find(1) + RUBY + end + it 'does not register an offense when using `find`' do expect_no_offenses(<<~RUBY) User.find(1) diff --git a/spec/rubocop/cop/rails/find_by_spec.rb b/spec/rubocop/cop/rails/find_by_spec.rb index ec6f9ff25c..ecf64ffb3b 100644 --- a/spec/rubocop/cop/rails/find_by_spec.rb +++ b/spec/rubocop/cop/rails/find_by_spec.rb @@ -12,6 +12,28 @@ RUBY end + it 'registers and corrects an offense when using `&.take`' do + expect_offense(<<~RUBY) + User.where(id: x)&.take + ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where&.take`. + RUBY + + expect_correction(<<~RUBY) + User.find_by(id: x) + RUBY + end + + it 'registers and corrects an offense when using `&.take` with safe navigation' do + expect_offense(<<~RUBY) + User&.where(id: x)&.take + ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where&.take`. + RUBY + + expect_correction(<<~RUBY) + User&.find_by(id: x) + RUBY + end + context 'when using safe navigation operator' do it 'registers an offense when using `#take`' do expect_offense(<<~RUBY) diff --git a/spec/rubocop/cop/rails/inquiry_spec.rb b/spec/rubocop/cop/rails/inquiry_spec.rb index 8ed0c736e7..be2caddfe9 100644 --- a/spec/rubocop/cop/rails/inquiry_spec.rb +++ b/spec/rubocop/cop/rails/inquiry_spec.rb @@ -8,6 +8,13 @@ RUBY end + it 'registers an offense when using `String&.inquiry`' do + expect_offense(<<~RUBY) + 'two'&.inquiry + ^^^^^^^ Prefer Ruby's comparison operators over Active Support's `inquiry`. + RUBY + end + it 'registers an offense when using `Array#inquiry`' do expect_offense(<<~RUBY) [foo, bar].inquiry diff --git a/spec/rubocop/cop/rails/pick_spec.rb b/spec/rubocop/cop/rails/pick_spec.rb index 3d17acde7e..c2bae0107e 100644 --- a/spec/rubocop/cop/rails/pick_spec.rb +++ b/spec/rubocop/cop/rails/pick_spec.rb @@ -3,7 +3,7 @@ RSpec.describe RuboCop::Cop::Rails::Pick, :config do context 'when using Rails 6.0 or newer', :rails60 do context 'with one argument' do - it 'registers an offense for `pluck(...).first' do + it 'registers an offense for `pluck(...).first`' do expect_offense(<<~RUBY) x.pluck(:a).first ^^^^^^^^^^^^^^^ Prefer `pick(:a)` over `pluck(:a).first`. @@ -13,10 +13,32 @@ x.pick(:a) RUBY end + + it 'registers an offense for `pluck(...)&.first`' do + expect_offense(<<~RUBY) + x.pluck(:a)&.first + ^^^^^^^^^^^^^^^^ Prefer `pick(:a)` over `pluck(:a)&.first`. + RUBY + + expect_correction(<<~RUBY) + x.pick(:a) + RUBY + end + + it 'registers an offense for `pluck(...)&.first` with safe navigation' do + expect_offense(<<~RUBY) + x&.pluck(:a)&.first + ^^^^^^^^^^^^^^^^ Prefer `pick(:a)` over `pluck(:a)&.first`. + RUBY + + expect_correction(<<~RUBY) + x&.pick(:a) + RUBY + end end context 'with multiple arguments' do - it 'registers an offense for `pluck(...).first' do + it 'registers an offense for `pluck(...).first`' do expect_offense(<<~RUBY) x.pluck(:a, :b).first ^^^^^^^^^^^^^^^^^^^ Prefer `pick(:a, :b)` over `pluck(:a, :b).first`. @@ -31,7 +53,7 @@ context 'when using Rails 5.2 or older', :rails52 do context 'with one argument' do - it 'does not register an offense for `pluck(...).first' do + it 'does not register an offense for `pluck(...).first`' do expect_no_offenses(<<~RUBY) x.pluck(:a).first RUBY @@ -39,7 +61,7 @@ end context 'with multiple arguments' do - it 'does not register an offense for `pluck(...).first' do + it 'does not register an offense for `pluck(...).first`' do expect_no_offenses(<<~RUBY) x.pluck(:a, :b).first RUBY diff --git a/spec/rubocop/cop/rails/pluck_id_spec.rb b/spec/rubocop/cop/rails/pluck_id_spec.rb index 58358f7c28..edeae8d234 100644 --- a/spec/rubocop/cop/rails/pluck_id_spec.rb +++ b/spec/rubocop/cop/rails/pluck_id_spec.rb @@ -12,6 +12,17 @@ RUBY end + it 'registers an offense and corrects when using `pluck(:id)` with safe navigation' do + expect_offense(<<~RUBY) + User&.pluck(:id) + ^^^^^^^^^^ Use `ids` instead of `pluck(:id)`. + RUBY + + expect_correction(<<~RUBY) + User&.ids + RUBY + end + it 'registers an offense and corrects when using `pluck(primary_key)`' do expect_offense(<<~RUBY) def self.user_ids diff --git a/spec/rubocop/cop/rails/pluck_in_where_spec.rb b/spec/rubocop/cop/rails/pluck_in_where_spec.rb index efa93a094c..bc7b0daa06 100644 --- a/spec/rubocop/cop/rails/pluck_in_where_spec.rb +++ b/spec/rubocop/cop/rails/pluck_in_where_spec.rb @@ -50,6 +50,17 @@ RUBY end + it 'registers an offense and corrects when using `pluck` in `where` for constant with safe navigation' do + expect_offense(<<~RUBY) + Post&.where(user_id: User&.active&.pluck(:id)) + ^^^^^ Use `select` instead of `pluck` within `where` query method. + RUBY + + expect_correction(<<~RUBY) + Post&.where(user_id: User&.active&.select(:id)) + RUBY + end + it 'registers an offense and corrects when using `pluck` in `rewhere` for constant' do expect_offense(<<~RUBY) Post.rewhere('user_id IN (?)', User.active.pluck(:id)) diff --git a/spec/rubocop/cop/rails/where_equals_spec.rb b/spec/rubocop/cop/rails/where_equals_spec.rb index 8bbea48233..69c718d85c 100644 --- a/spec/rubocop/cop/rails/where_equals_spec.rb +++ b/spec/rubocop/cop/rails/where_equals_spec.rb @@ -12,6 +12,17 @@ RUBY end + it 'registers an offense and corrects when using `=` and anonymous placeholder with safe navigation' do + expect_offense(<<~RUBY) + User&.where('name = ?', 'Gabe') + ^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User&.where(name: 'Gabe') + RUBY + end + it 'registers an offense and corrects when using `=` and named placeholder' do expect_offense(<<~RUBY) User.where('name = :name', name: 'Gabe') @@ -79,6 +90,17 @@ RUBY end + it 'registers an offense and corrects when using `=` and anonymous placeholder with safe navigation' do + expect_offense(<<~RUBY) + User&.where(['name = ?', 'Gabe']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL. + RUBY + + expect_correction(<<~RUBY) + User&.where(name: 'Gabe') + RUBY + end + it 'registers an offense and corrects when using `=` and named placeholder' do expect_offense(<<~RUBY) User.where(['name = :name', { name: 'Gabe' }]) diff --git a/spec/rubocop/cop/rails/where_exists_spec.rb b/spec/rubocop/cop/rails/where_exists_spec.rb index e98332c27f..b16a3e023d 100644 --- a/spec/rubocop/cop/rails/where_exists_spec.rb +++ b/spec/rubocop/cop/rails/where_exists_spec.rb @@ -15,6 +15,28 @@ RUBY end + it 'registers an offense and corrects when using `where(...)&.exists?` with hash argument' do + expect_offense(<<~RUBY) + User.where(name: 'john')&.exists? + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `exists?(name: 'john')` over `where(name: 'john')&.exists?`. + RUBY + + expect_correction(<<~RUBY) + User.exists?(name: 'john') + RUBY + end + + it 'registers an offense and corrects when using `where(...)&.exists?` with hash argument with safe navigation' do + expect_offense(<<~RUBY) + User&.where(name: 'john')&.exists? + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `exists?(name: 'john')` over `where(name: 'john')&.exists?`. + RUBY + + expect_correction(<<~RUBY) + User&.exists?(name: 'john') + RUBY + end + it 'registers an offense and corrects when using `where(...).exists?` with array argument' do expect_offense(<<~RUBY) User.where(['name = ?', 'john']).exists? @@ -87,6 +109,17 @@ RUBY end + it 'registers an offense and corrects when using `exists?` with a hash with safe navigation' do + expect_offense(<<~RUBY) + User&.exists?(name: 'john') + ^^^^^^^^^^^^^^^^^^^^^ Prefer `where(name: 'john')&.exists?` over `exists?(name: 'john')`. + RUBY + + expect_correction(<<~RUBY) + User&.where(name: 'john')&.exists? + RUBY + end + it 'registers an offense and corrects when using `exists?` with an array' do expect_offense(<<~RUBY) User.exists?(['name = ?', 'john']) diff --git a/spec/rubocop/cop/rails/where_not_spec.rb b/spec/rubocop/cop/rails/where_not_spec.rb index 4ce3d88fdb..aa257d7246 100644 --- a/spec/rubocop/cop/rails/where_not_spec.rb +++ b/spec/rubocop/cop/rails/where_not_spec.rb @@ -12,6 +12,17 @@ RUBY end + it 'registers an offense and corrects when using `!=` and anonymous placeholder with safe navigation' do + expect_offense(<<~RUBY) + User&.where('name != ?', 'Gabe') + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where&.not(name: 'Gabe')` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User&.where&.not(name: 'Gabe') + RUBY + end + it 'registers an offense and corrects when using `!=` and named placeholder' do expect_offense(<<~RUBY) User.where('name != :name', name: 'Gabe') @@ -112,6 +123,17 @@ RUBY end + it 'registers an offense and corrects when using `!=` and anonymous placeholder with safe navigation' do + expect_offense(<<~RUBY) + User&.where(['name != ?', 'Gabe']) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where&.not(name: 'Gabe')` instead of manually constructing negated SQL in `where`. + RUBY + + expect_correction(<<~RUBY) + User&.where&.not(name: 'Gabe') + RUBY + end + it 'registers an offense and corrects when using `!=` and named placeholder' do expect_offense(<<~RUBY) User.where(['name != :name', { name: 'Gabe' }])