diff --git a/changelog/change_presence_to_support_chained_calls.md b/changelog/change_presence_to_support_chained_calls.md new file mode 100644 index 0000000000..ae2b7f43b3 --- /dev/null +++ b/changelog/change_presence_to_support_chained_calls.md @@ -0,0 +1 @@ +* [#932](https://github.com/rubocop/rubocop-rails/issues/932): Add support for chained method calls in `Rails/Presence`. ([@vlad-pisanov][]) diff --git a/config/default.yml b/config/default.yml index 61fb424317..99f52e49f9 100644 --- a/config/default.yml +++ b/config/default.yml @@ -809,6 +809,7 @@ Rails/Presence: Description: 'Checks code that can be written more easily using `Object#presence` defined by Active Support.' Enabled: true VersionAdded: '0.52' + VersionChanged: '<>' Rails/Present: Description: 'Enforces use of `present?`.' diff --git a/lib/rubocop/cop/rails/presence.rb b/lib/rubocop/cop/rails/presence.rb index 2bfceb1d08..e612eaf6ca 100644 --- a/lib/rubocop/cop/rails/presence.rb +++ b/lib/rubocop/cop/rails/presence.rb @@ -37,6 +37,22 @@ module Rails # # # good # a.presence || b + # + # @example + # # bad + # a.present? ? a.foo : nil + # + # # bad + # !a.present? ? nil : a.foo + # + # # bad + # a.blank? ? nil : a.foo + # + # # bad + # !a.blank? ? a.foo : nil + # + # # good + # a.presence&.foo class Presence < Base include RangeHelp extend AutoCorrector @@ -46,29 +62,29 @@ class Presence < Base def_node_matcher :redundant_receiver_and_other, <<~PATTERN { (if - (send $_recv :present?) - _recv + {(send $_recv :blank?) (send (send $_recv :present?) :!)} $!begin + _recv ) (if - (send $_recv :blank?) - $!begin + {(send $_recv :present?) (send (send $_recv :blank?) :!)} _recv + $!begin ) } PATTERN - def_node_matcher :redundant_negative_receiver_and_other, <<~PATTERN + def_node_matcher :redundant_receiver_and_chain, <<~PATTERN { (if - (send (send $_recv :present?) :!) - $!begin - _recv + {(send $_recv :blank?) (send (send $_recv :present?) :!)} + {nil? nil_type?} + $(send _recv ...) ) (if - (send (send $_recv :blank?) :!) - _recv - $!begin + {(send $_recv :present?) (send (send $_recv :blank?) :!)} + $(send _recv ...) + {nil? nil_type?} ) } PATTERN @@ -82,18 +98,26 @@ def on_if(node) register_offense(node, receiver, other) end - redundant_negative_receiver_and_other(node) do |receiver, other| - return if ignore_other_node?(other) || receiver.nil? + redundant_receiver_and_chain(node) do |receiver, chain| + return if ignore_chain_node?(chain) || receiver.nil? - register_offense(node, receiver, other) + register_chain_offense(node, receiver, chain) end end private def register_offense(node, receiver, other) - add_offense(node, message: message(node, receiver, other)) do |corrector| - corrector.replace(node, replacement(receiver, other, node.left_sibling)) + replacement = replacement(receiver, other, node.left_sibling) + add_offense(node, message: message(node, replacement)) do |corrector| + corrector.replace(node, replacement) + end + end + + def register_chain_offense(node, receiver, chain) + replacement = chain_replacement(receiver, chain, node.left_sibling) + add_offense(node, message: message(node, replacement)) do |corrector| + corrector.replace(node, replacement) end end @@ -105,8 +129,12 @@ def ignore_other_node?(node) node&.type?(:if, :rescue, :while) end - def message(node, receiver, other) - prefer = replacement(receiver, other, node.left_sibling).gsub(/^\s*|\n/, '') + def ignore_chain_node?(node) + node.method?('[]') || node.arithmetic_operation? + end + + def message(node, replacement) + prefer = replacement.gsub(/^\s*|\n/, '') current = current(node).gsub(/^\s*|\n/, '') format(MSG, prefer: prefer, current: current) end @@ -146,6 +174,12 @@ def build_source_for_or_method(other) def method_range(node) range_between(node.source_range.begin_pos, node.first_argument.source_range.begin_pos - 1) end + + def chain_replacement(receiver, chain, left_sibling) + replaced = "#{receiver.source}.presence&.#{chain.method_name}" + replaced += "(#{chain.arguments.map(&:source).join(', ')})" if chain.arguments? + left_sibling ? "(#{replaced})" : replaced + end end end end diff --git a/spec/rubocop/cop/rails/presence_spec.rb b/spec/rubocop/cop/rails/presence_spec.rb index 657039d1a5..433c8ca3b2 100644 --- a/spec/rubocop/cop/rails/presence_spec.rb +++ b/spec/rubocop/cop/rails/presence_spec.rb @@ -233,6 +233,81 @@ RUBY end + context 'when a method is called on the receiver' do + it 'registers an offense and corrects when `a.present? ? a.foo : nil' do + expect_offense(<<~RUBY) + a.present? ? a.foo : nil + ^^^^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.present? ? a.foo : nil`. + RUBY + + expect_correction(<<~RUBY) + a.presence&.foo + RUBY + end + + it 'registers an offense and corrects when `a.blank? ? nil : a.foo' do + expect_offense(<<~RUBY) + a.blank? ? nil : a.foo + ^^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.blank? ? nil : a.foo`. + RUBY + + expect_correction(<<~RUBY) + a.presence&.foo + RUBY + end + + it 'registers an offense and corrects when `a.foo if a.present?`' do + expect_offense(<<~RUBY) + a.foo if a.present? + ^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.foo if a.present?`. + RUBY + + expect_correction(<<~RUBY) + a.presence&.foo + RUBY + end + + it 'registers an offense and corrects when `a.foo unless a.blank?`' do + expect_offense(<<~RUBY) + a.foo unless a.blank? + ^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo` instead of `a.foo unless a.blank?`. + RUBY + + expect_correction(<<~RUBY) + a.presence&.foo + RUBY + end + + it 'registers an offense and corrects when chained method takes parameters' do + expect_offense(<<~RUBY) + a.present? ? a.foo(42, key: :value) : nil + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `a.presence&.foo(42, key: :value)` instead of `a.present? ? a.foo(42, key: :value) : nil`. + RUBY + + expect_correction(<<~RUBY) + a.presence&.foo(42, key: :value) + RUBY + end + + it 'does not register an offense when chained method is `[]`' do + expect_no_offenses(<<~RUBY) + a.present? ? a[1] : nil + RUBY + end + + it 'does not register an offense when chained method is an arithmetic operation' do + expect_no_offenses(<<~RUBY) + a.present? ? a + 42 : nil + RUBY + end + + it 'does not register an offense when multiple methods are chained' do + expect_no_offenses(<<~RUBY) + a.present? ? a.foo.bar : nil + RUBY + end + end + context 'when multiline ternary can be replaced' do it 'registers an offense and corrects' do expect_offense(<<~RUBY)