Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make some cops aware of safe navigation operator #1204

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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][])
2 changes: 1 addition & 1 deletion lib/rubocop/cop/mixin/active_record_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 6 additions & 5 deletions lib/rubocop/cop/rails/active_support_aliases.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/rails/find_by.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class FindBy < Base
include RangeHelp
extend AutoCorrector

MSG = 'Use `find_by` instead of `where.%<method>s`.'
MSG = 'Use `find_by` instead of `where%<dot>s%<method>s`.'
RESTRICT_ON_SEND = %i[first take].freeze

def on_send(node)
Expand All @@ -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
Expand Down
32 changes: 9 additions & 23 deletions lib/rubocop/cop/rails/find_by_id.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails/inquiry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ def on_send(node)

add_offense(node.loc.selector)
end
alias on_csend on_send
end
end
end
Expand Down
11 changes: 6 additions & 5 deletions lib/rubocop/cop/rails/pick.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ class Pick < Base
extend AutoCorrector
extend TargetRailsVersion

MSG = 'Prefer `pick(%<args>s)` over `pluck(%<args>s).first`.'
MSG = 'Prefer `pick(%<args>s)` over `%<current>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)
Expand All @@ -44,19 +44,20 @@ 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)
corrector.replace(receiver_selector, 'pick')
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
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails/pluck_id.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -47,6 +47,7 @@ def on_send(node)
corrector.replace(offense_range(node), 'ids')
end
end
alias on_csend on_send

private

Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails/pluck_in_where.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions lib/rubocop/cop/rails/where_equals.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 (?)
Expand Down
17 changes: 9 additions & 8 deletions lib/rubocop/cop/rails/where_exists.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,27 @@ 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)
find_offenses(node) do |args|
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|
corrector.replace(range, good_method)
end
end
end
alias on_csend on_send

private

Expand Down Expand Up @@ -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

Expand All @@ -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
Expand Down
13 changes: 7 additions & 6 deletions lib/rubocop/cop/rails/where_not.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -46,14 +46,15 @@ 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|
corrector.replace(range, good_method)
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 (?)
Expand Down Expand Up @@ -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
Expand Down
Loading