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

[Fix #103] Fix a false positive for Rails/FindEach #464

Merged
merged 1 commit into from
Apr 26, 2021
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* [#435](https://github.com/rubocop/rubocop-rails/issues/435): Fix a false negative for `Rails/BelongsTo` when using `belongs_to` lambda block with `required` option. ([@koic][])
* [#451](https://github.com/rubocop/rubocop-rails/issues/451): Fix a false negative for `Rails/RelativeDateConstant` when a method is chained after a relative date method. ([@koic][])
* [#450](https://github.com/rubocop/rubocop-rails/issues/450): Fix a crash for `Rails/ContentTag` with nested content tags. ([@tejasbubane][])
* [#103](https://github.com/rubocop/rubocop-rails/issues/103): Fix a false positive for `Rails/FindEach` when not inheriting `ActiveRecord::Base` and using `all.each`. ([@koic][])

### Changes

Expand Down
11 changes: 11 additions & 0 deletions lib/rubocop/cop/mixin/active_record_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@ module ActiveRecordHelper

WHERE_METHODS = %i[where rewhere].freeze

def_node_matcher :active_record?, <<~PATTERN
{
(const nil? :ApplicationRecord)
(const (const nil? :ActiveRecord) :Base)
}
PATTERN

def_node_search :find_set_table_name, <<~PATTERN
(send self :table_name= {str sym})
PATTERN
Expand All @@ -16,6 +23,10 @@ module ActiveRecordHelper
(send nil? :belongs_to {str sym} ...)
PATTERN

def inherit_active_record_base?(node)
node.each_ancestor(:class).any? { |class_node| active_record?(class_node.parent_class) }
end

def external_dependency_checksum
return @external_dependency_checksum if defined?(@external_dependency_checksum)

Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails/find_each.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module Rails
# # good
# User.order(:foo).each
class FindEach < Base
include ActiveRecordHelper
extend AutoCorrector

MSG = 'Use `find_each` instead of `each`.'
Expand All @@ -30,6 +31,7 @@ class FindEach < Base
def on_send(node)
return unless node.receiver&.send_type?
return unless SCOPE_METHODS.include?(node.receiver.method_name)
return if node.receiver.receiver.nil? && !inherit_active_record_base?(node)
return if ignored?(node)

range = node.loc.selector
Expand Down
36 changes: 36 additions & 0 deletions spec/rubocop/cop/rails/find_each_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,42 @@ class C; User.all.find_each { |u| u.x }; end
RUBY
end

context 'with no receiver' do
it 'does not register an offense when not inheriting any class' do
expect_no_offenses(<<~RUBY)
class C
all.each { |u| u.x }
end
RUBY
end

it 'does not register an offense when not inheriting `ApplicationRecord`' do
expect_no_offenses(<<~RUBY)
class C < Foo
all.each { |u| u.x }
end
RUBY
end

it 'registers an offense when inheriting `ApplicationRecord`' do
expect_offense(<<~RUBY)
class C < ApplicationRecord
all.each { |u| u.x }
^^^^ Use `find_each` instead of `each`.
end
RUBY
end

it 'registers an offense when inheriting `ActiveRecord::Base`' do
expect_offense(<<~RUBY)
class C < ActiveRecord::Base
all.each { |u| u.x }
^^^^ Use `find_each` instead of `each`.
end
RUBY
end
end

context 'ignored methods' do
let(:cop_config) { { 'IgnoredMethods' => %w[order lock] } }

Expand Down