Skip to content

Commit

Permalink
Fix a false positive for Rails/DynamicFindBy
Browse files Browse the repository at this point in the history
Fixes rubocop/rubocop-jp#54 (Ja)

This PR fixes a false positive for `Rails/DynamicFindBy`
when not inheriting `ApplicationRecord` and without no receiver.

A dynamic matcher without receiver is only possible if it inherits
`ApplicationRecord` (`ActiveRecord::Base`).

For example, Capybara's `find_by_id` and Token's `find_by_valid_token`
should be accepted by default.

- https://www.rubydoc.info/github/jnicklas/capybara/Capybara/Node/Finders#find_by_id-instance_method
- https://github.com/fnando/tokens
  • Loading branch information
koic committed Apr 26, 2021
1 parent 8151200 commit dc6b3cb
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* [#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][])
* [#466](https://github.com/rubocop/rubocop-rails/pull/466): Fix a false positive for `Rails/DynamicFindBy` when not inheriting `ApplicationRecord` and without no receiver. ([@koic][])

### Changes

Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails/dynamic_find_by.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ module Rails
# # good
# Gem::Specification.find_by_name('backend').gem_dir
class DynamicFindBy < Base
include ActiveRecordHelper
extend AutoCorrector

MSG = 'Use `%<static_name>s` instead of dynamic `%<method>s`.'
METHOD_PATTERN = /^find_by_(.+?)(!)?$/.freeze

def on_send(node)
return if allowed_invocation?(node)
return if node.receiver.nil? && !inherit_active_record_base?(node) || allowed_invocation?(node)

method_name = node.method_name
static_name = static_method_name(method_name)
Expand Down
44 changes: 44 additions & 0 deletions spec/rubocop/cop/rails/dynamic_find_by_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,50 @@
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
def do_something
find_by_name(name)
end
end
RUBY
end

it 'does not register an offense when not inheriting `ApplicationRecord`' do
expect_no_offenses(<<~RUBY)
class C < Foo
def do_something
find_by_name(name)
end
end
RUBY
end

it 'registers an offense when inheriting `ApplicationRecord`' do
expect_offense(<<~RUBY)
class C < ApplicationRecord
def do_something
find_by_name(name)
^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
end
end
RUBY
end

it 'registers an offense when inheriting `ActiveRecord::Base`' do
expect_offense(<<~RUBY)
class C < ActiveRecord::Base
def do_something
find_by_name(name)
^^^^^^^^^^^^^^^^^^ Use `find_by` instead of dynamic `find_by_name`.
end
end
RUBY
end
end

context 'with allowed receiver name' do
let(:cop_config) do
{ 'AllowedReceivers' => %w[Gem::Specification] }
Expand Down

0 comments on commit dc6b3cb

Please sign in to comment.