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/FindBy doesn't check for where in the method chain #504

Closed
nvasilevski opened this issue Jun 9, 2021 · 5 comments
Closed

Rails/FindBy doesn't check for where in the method chain #504

nvasilevski opened this issue Jun 9, 2021 · 5 comments

Comments

@nvasilevski
Copy link
Contributor

Most likely bug was introduced here - #502 by removing the :where_first? matcher
cc: @koic

Expected behavior

Rails/FindBy cop should only look for .where().take or where().first calls

Actual behavior

Rails/FindBy just looks for any .take or .first calls

Steps to reproduce the problem

The following test should fail but it passes with a misleading suggestion:

# spec/rubocop/cop/rails/find_by_spec.rb
  it 'wrongly expects offence for a regular array' do
    expect_offense(<<~RUBY)
      array = Array.new(1) { rand }
      array.compact.take
            ^^^^^^^^^^^^ Use `find_by` instead of `where.take`.
    RUBY
  end

RuboCop version

[ruby-2.7.1p83] (master) rubocop-rails ツ bundle exec rubocop -V
1.16.1 (using Parser 3.0.1.1, rubocop-ast 1.7.0, running on ruby 2.7.1 x86_64-darwin19)
  - rubocop-performance 1.11.3
  - rubocop-rails 2.10.1
  - rubocop-rspec 1.29.1
@koic
Copy link
Member

koic commented Jun 10, 2021

#502 has not been release yet. I'm spending some time working for the next release. Thank you for your patience.

@nvasilevski
Copy link
Contributor Author

Got it, thanks!
Just wanted to mention that I don't mind fixing it by myself, so basically created this issue to make sure that I'm not mistaken and this is something we actually want to address

@koic
Copy link
Member

koic commented Jun 21, 2021

RuboCop Rails 2.11 is out! Thank you for waiting!
https://github.com/rubocop/rubocop-rails/releases/tag/v2.11.0

@koic koic closed this as completed Jun 21, 2021
@nvasilevski
Copy link
Contributor Author

Hey @koic, thanks for releasing the version
But I still can reproduce the unexpected behaviour:
I'm running on

[ruby-2.7.1p83] ( ±) rubocop-rails ツ bundle exec rubocop -V
1.16.1 (using Parser 3.0.1.1, rubocop-ast 1.7.0, running on ruby 2.7.1 x86_64-darwin19)
  - rubocop-performance 1.11.3
  - rubocop-rails 2.11.0
  - rubocop-rspec 1.29.1

And the test

# spec/rubocop/cop/rails/find_by_spec.rb
  it 'wrongly expects offence for a regular array' do
    expect_offense(<<~RUBY)
      array = Array.new(1) { rand }
      array.compact.take
            ^^^^^^^^^^^^ Use `find_by` instead of `where.take`.
    RUBY
  end

Still passes, but it should fail instead. We don't want to have an offence with regular ruby arrays. There is no find_by method available for an array.

Am I missing something? Thanks! ❤️

@nvasilevski
Copy link
Contributor Author

Here is an example of the fix I meant - #506

nvasilevski added a commit to nvasilevski/rubocop-rails that referenced this issue Jun 21, 2021
nvasilevski added a commit to nvasilevski/rubocop-rails that referenced this issue Jun 22, 2021
koic added a commit that referenced this issue Jun 23, 2021
…triggering-on-regular-arrays

[Fix #504] Rails/FindBy being wrongly triggered on non Active Record methods
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

No branches or pull requests

2 participants