Skip to content
New issue

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

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

Already on GitHub? # 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`
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants