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

Performance/AncestorsInclude shows warning for instance methods #ancestors #159

Closed
klyonrad opened this issue Aug 26, 2020 · 1 comment · Fixed by #160
Closed

Performance/AncestorsInclude shows warning for instance methods #ancestors #159

klyonrad opened this issue Aug 26, 2020 · 1 comment · Fixed by #160

Comments

@klyonrad
Copy link

klyonrad commented Aug 26, 2020

Calling a method with the name ancestors on an object raises the rubocop-performance warning.

Is this possible to detect more correctly with static code analysis or are we out of luck? If not, honestly I would question the usefulness of this cop because I don't think we are the only ones to happen to have data models where something is called ancestors.


Expected behavior

rubocop-performance only warns when .ancestors is called on a Class.
Calling of the instance method should be fine

Actual behavior

It seems to screem at any calling of something called .ancestors

Steps to reproduce the problem

We have a rails model with the following association declaration

has_many :ancestors

Somewhere we have a test where usage this method - on the instance.

expect(object_one.ancestors.include?(object_two)).to eq(true)

rubocop warns

Performance/AncestorsInclude: Use <= instead of ancestors.include?

RuboCop version

$ [bundle exec] rubocop -V
0.83.0 (using Parser 2.7.1.4, running on ruby 2.6.6 x86_64-darwin19)
# ...
ruby-2.6.6/gems/rubocop-performance-1.7.1
koic added a commit to koic/rubocop-performance that referenced this issue Aug 26, 2020
…ude`

Fixes rubocop#159.

This PR fixes a false positive for `Performance/AncestorsInclude`
when receiver is a variable.

As the cop's example shows, this change mainly targets when the receiver
is a constant. It can cause false negatives, but it may be less than
false positives.
And this change may lead to removing unsafe marked by rubocop#149.
@marcandre
Copy link
Contributor

@koic is working on a PR, but another way around is that this test could be improved to:

expect(object_one.ancestors).to include(object_two)

😄

@koic koic closed this as completed in #160 Aug 30, 2020
koic added a commit that referenced this issue Aug 30, 2020
…ancestors_include

[Fix #159] Fix a false positive for `Performance/AncestorsInclude`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants