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

Expand Rails/FindBy (and FindEach) to cover all code locations #1058

Closed
gravitystorm opened this issue Aug 2, 2023 · 0 comments · Fixed by #1059
Closed

Expand Rails/FindBy (and FindEach) to cover all code locations #1058

gravitystorm opened this issue Aug 2, 2023 · 0 comments · Fixed by #1059

Comments

@gravitystorm
Copy link

Is your feature request related to a problem? Please describe.

I was trying to figure out why rubocop-rails wasn't picking up where(...).first usage in our application (OpenStreetMap).

Eventually I found that the cop is restricted to models only, in the default configuration.

Rails/FindBy:
Description: 'Prefer find_by over where.first.'
StyleGuide: 'https://rails.rubystyle.guide#find_by'
Enabled: true
VersionAdded: '0.30'
VersionChanged: '2.11'
IgnoreWhereFirst: true
Include:
- app/models/**/*.rb

However I don't think this restriction is necessary, and I think it's actually unhelpful. I suspect that a large amount, perhaps the majority, of ActiveRecord .where usage would be found in controllers, not in models. In our application, it also crops up repeatedly in our tests.

Describe the solution you'd like

I'd like to see the default configuration updated, to remove the Include: ['app/models/**/*.rb'] restriction, for the Rails/FindBy and Rails/FindEach cops.

Describe alternatives you've considered

The alternative is for everyone to override the configuration for these two cops in their own configuration, e.g.

Rails/FindBy:
  Include:
    - '**/*.rb'

Additional context

The Include restriction was originally introduced alongside the initial cop (in rubocop) in 6188d755eefd94875bc155bbfe433f7752d0bd2b, but I can't find any related discussions.

koic added a commit to koic/rubocop-rails that referenced this issue Aug 2, 2023
…/FindEach`

Fixes rubocop#1058.

This PR relaxes `Include` path for `Rails/FindBy` and `Rails/FindEach`.

`Rails/FindBy` was introduced in rubocop/rubocop@6188d75.
This was before it was extracted to RuboCop Rails, which is probably why it was
targeting `app/models`. So, RuboCop Rails targeting Rails would not need it.

`Rails/FindEach` is the same:
rubocop/rubocop@f2e26812
@koic koic closed this as completed in #1059 Aug 3, 2023
koic added a commit that referenced this issue Aug 3, 2023
…by_and_find_each

[Fix #1058] Relax `Include` path for `Rails/FindBy` and `Rails/FindEach`
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.

1 participant