From 3cefa3ddd5261dbe1a036c8e56899a23a1f89c23 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 3 Jun 2021 11:38:57 +0900 Subject: [PATCH] [Fix #499] Add `IgnoreWhereFirst` to `Rails/FindBy` cop Fixes #499. This PR add `IgnoreWhereFirst` option (`true` by default) to `Rails/FindBy` cop. --- CHANGELOG.md | 1 + config/default.yml | 2 + docs/modules/ROOT/pages/cops_rails.adoc | 31 ++++++++++++-- lib/rubocop/cop/rails/find_by.rb | 30 +++++++++----- spec/rubocop/cop/rails/find_by_spec.rb | 54 +++++++++++++++---------- 5 files changed, 81 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e615dc5e3c..16c2ab95d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ ### Changes * [#288](https://github.com/rubocop/rubocop-rails/issues/288): Add `AllowToTime` option (`true` by default) to `Rails/Date`. ([@koic][]) +* [#499](https://github.com/rubocop/rubocop-rails/issues/499): Add `IgnoreWhereFirst` option (`true` by default) to `Rails/FindBy`. ([@koic][]) ## 2.10.1 (2021-05-06) diff --git a/config/default.yml b/config/default.yml index e7ce86c7e8..8d40aeb40a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -307,6 +307,8 @@ Rails/FindBy: StyleGuide: 'https://rails.rubystyle.guide#find_by' Enabled: true VersionAdded: '0.30' + VersionChanged: '2.11' + IgnoreWhereFirst: true Include: - app/models/**/*.rb diff --git a/docs/modules/ROOT/pages/cops_rails.adoc b/docs/modules/ROOT/pages/cops_rails.adoc index 9dde279015..34afc51a9f 100644 --- a/docs/modules/ROOT/pages/cops_rails.adoc +++ b/docs/modules/ROOT/pages/cops_rails.adoc @@ -1540,29 +1540,52 @@ Rails.root.join('app/models/goober') | Yes | Yes | 0.30 -| - +| 2.11 |=== -This cop is used to identify usages of `where.first` and -change them to use `find_by` instead. +This cop is used to identify usages of `where.take` and change them to use `find_by` instead. + +And `where(...).first` can return different results from `find_by`. +(They order records differently, so the "first" record can be different.) + +If you also want to detect `where.first`, you can set `IgnoreWhereFirst` to false. === Examples [source,ruby] ---- # bad -User.where(name: 'Bruce').first User.where(name: 'Bruce').take # good User.find_by(name: 'Bruce') ---- +==== IgnoreWhereFirst: true (default) + +[source,ruby] +---- +# bad +User.where(name: 'Bruce').first +---- + +==== IgnoreWhereFirst: false + +[source,ruby] +---- +# good +User.where(name: 'Bruce').first +---- + === Configurable attributes |=== | Name | Default value | Configurable values +| IgnoreWhereFirst +| `true` +| Boolean + | Include | `app/models/**/*.rb` | Array diff --git a/lib/rubocop/cop/rails/find_by.rb b/lib/rubocop/cop/rails/find_by.rb index aa58f63371..3d39193bed 100644 --- a/lib/rubocop/cop/rails/find_by.rb +++ b/lib/rubocop/cop/rails/find_by.rb @@ -3,16 +3,27 @@ module RuboCop module Cop module Rails - # This cop is used to identify usages of `where.first` and - # change them to use `find_by` instead. + # This cop is used to identify usages of `where.take` and change them to use `find_by` instead. + # + # And `where(...).first` can return different results from `find_by`. + # (They order records differently, so the "first" record can be different.) + # + # If you also want to detect `where.first`, you can set `IgnoreWhereFirst` to false. # # @example # # bad - # User.where(name: 'Bruce').first # User.where(name: 'Bruce').take # # # good # User.find_by(name: 'Bruce') + # + # @example IgnoreWhereFirst: true (default) + # # bad + # User.where(name: 'Bruce').first + # + # @example IgnoreWhereFirst: false + # # good + # User.where(name: 'Bruce').first class FindBy < Base include RangeHelp extend AutoCorrector @@ -20,12 +31,8 @@ class FindBy < Base MSG = 'Use `find_by` instead of `where.%s`.' RESTRICT_ON_SEND = %i[first take].freeze - def_node_matcher :where_first?, <<~PATTERN - (send ({send csend} _ :where ...) {:first :take}) - PATTERN - def on_send(node) - return unless where_first?(node) + return if ignore_where_first? && node.method?(:first) range = range_between(node.receiver.loc.selector.begin_pos, node.loc.selector.end_pos) @@ -38,9 +45,6 @@ def on_send(node) private def autocorrect(corrector, node) - # Don't autocorrect where(...).first, because it can return different - # results from find_by. (They order records differently, so the - # 'first' record can be different.) return if node.method?(:first) where_loc = node.receiver.loc.selector @@ -49,6 +53,10 @@ def autocorrect(corrector, node) corrector.replace(where_loc, 'find_by') corrector.replace(first_loc, '') end + + def ignore_where_first? + cop_config.fetch('IgnoreWhereFirst', true) + end end end end diff --git a/spec/rubocop/cop/rails/find_by_spec.rb b/spec/rubocop/cop/rails/find_by_spec.rb index f6d078944e..fb80730cdd 100644 --- a/spec/rubocop/cop/rails/find_by_spec.rb +++ b/spec/rubocop/cop/rails/find_by_spec.rb @@ -1,16 +1,7 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Rails::FindBy, :config do - it 'registers an offense when using `#first` and does not auto-correct' do - expect_offense(<<~RUBY) - User.where(id: x).first - ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where.first`. - RUBY - - expect_no_corrections - end - - it 'registers an offense when using `#take`' do + it 'registers and corrects an offense when using `#take`' do expect_offense(<<~RUBY) User.where(id: x).take ^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where.take`. @@ -21,28 +12,47 @@ RUBY end + context 'when using safe navigation operator' do + it 'registers an offense when using `#take`' do + expect_offense(<<~RUBY) + users&.where(id: x).take + ^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where.take`. + RUBY + + expect_correction(<<~RUBY) + users&.find_by(id: x) + RUBY + end + end + it 'does not register an offense when using find_by' do expect_no_offenses('User.find_by(id: x)') end - it 'autocorrects where.take to find_by' do - new_source = autocorrect_source('User.where(id: x).take') + context 'when `IgnoreWhereFirst: true' do + let(:cop_config) do + { 'IgnoreWhereFirst' => true } + end - expect(new_source).to eq('User.find_by(id: x)') + it 'does not register an offense when using `#first`' do + expect_no_offenses(<<~RUBY) + User.where(id: x).first + RUBY + end end - it 'does not autocorrect where.first' do - new_source = autocorrect_source('User.where(id: x).first') - - expect(new_source).to eq('User.where(id: x).first') - end + context 'when `IgnoreWhereFirst: false' do + let(:cop_config) do + { 'IgnoreWhereFirst' => false } + end - context 'when using safe navigation operator' do - it 'registers an offense when using `#first`' do + it 'registers an offense when using `#first` and does not auto-correct' do expect_offense(<<~RUBY) - User&.where(id: x).first - ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where.first`. + User.where(id: x).first + ^^^^^^^^^^^^^^^^^^ Use `find_by` instead of `where.first`. RUBY + + expect_no_corrections end end end