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

Rails/DynamicFindBy false-positives on Gem::Specification.find_by_name #4967

Closed
tobypinder opened this issue Oct 30, 2017 · 2 comments
Closed
Labels

Comments

@tobypinder
Copy link

tobypinder commented Oct 30, 2017

Gem::Specification.find_by_name clearly looks like ActiveRecord to this cop but isn't.

False positives are inevitable and whitelist functionality has been explained before, but given this class is in rubygems itself it would feel prudent to add Gem::Specification#find_by_name and Gem::Specification#find_by_path to a default or even hardcoded whitelist inside the cop, as I cannot think of any scenario other than a namespace collision where detecting these as activerecord is expected behaviour.


Expected behavior

No offences found.

Actual behavior

Rails/DynamicFindBy: Use 'find_by' instead of dynamic 'find_by_name'. (Rails/DynamicFindBy)

Steps to reproduce the problem

Gem::Specification.find_by_name('loremipsum')

RuboCop version

$ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.3.5 x86_64-linux)
@pocke pocke added the bug label Nov 1, 2017
koic added a commit to koic/rubocop that referenced this issue Nov 4, 2017
…ctiveRecord

Fixes rubocop#4967.
This change applies `Rails/DynamicFindBy` cop only to receiver classes
that inherit `ActiveRecord::Base` class.
@tobypinder
Copy link
Author

tobypinder commented Nov 13, 2017

I see the comments from the @koic's attempt at resolving this. Would it be possible to expand the Whitelist functionality from method to ClassName#instance_method / ClassName.class_method?

I'm admittedly not at all familiar with rubocop's internals and such an approach doesn't deal with subclasses as @koic attempted but surely the class name itself is available to the AST, and we're not particularly worried about Gem::Specification getting subclassed that much? Failing that can we whitelist whole classes under a different key?

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 19, 2018

We're in the process of moving all Rails-related functionality to a standalone library (gem) named rubocop-rails. Please, migrate this issue to its issue tracker https://github.com/rubocop-hq/rubocop-rails/issues/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants