Merge pull request #581 from pirj/improve-uniq_before_pluck-examples

Iron out Rails/UniqBeforePluck

Author: koic

docs/modules/ROOT/pages/cops_rails.adoc

@@ -4912,58 +4912,56 @@ end
 | 2.13
 |===
 
-Prefer the use of distinct, before pluck instead of after.
+Prefer using `distinct` before `pluck` instead of `uniq` after `pluck`.
 
-The use of distinct before pluck is preferred because it executes within
+The use of distinct before pluck is preferred because it executes by
 the database.
 
 This cop has two different enforcement modes. When the EnforcedStyle
-is conservative (the default) then only calls to pluck on a constant
-(i.e. a model class) before distinct are added as offenses.
+is `conservative` (the default), then only calls to `pluck` on a constant
+(i.e. a model class) before `uniq` are added as offenses.
 
-When the EnforcedStyle is aggressive then all calls to pluck before
+When the EnforcedStyle is `aggressive` then all calls to `pluck` before
 distinct are added as offenses. This may lead to false positives
-as the cop cannot distinguish between calls to pluck on an
+as the cop cannot distinguish between calls to `pluck` on an
 ActiveRecord::Relation vs a call to pluck on an
 ActiveRecord::Associations::CollectionProxy.
 
 === Safety
 
-This cop is unsafe because the behavior may change depending on the
-database collation.
-Autocorrect is disabled by default for this cop since it may generate
-false positives.
+This cop is unsafe for autocorrection because the behavior may change
+depending on the database collation.
 
 === Examples
 
 ==== EnforcedStyle: conservative (default)
 
 [source,ruby]
 ----
-# bad
-Model.pluck(:id).uniq
+# bad - redundantly fetches duplicate values
+Album.pluck(:band_name).uniq
 
 # good
-Model.distinct.pluck(:id)
+Album.distinct.pluck(:band_name)
 ----
 
 ==== EnforcedStyle: aggressive
 
 [source,ruby]
 ----
-# bad
-# this will return a Relation that pluck is called on
-Model.where(cond: true).pluck(:id).uniq
+# bad - redundantly fetches duplicate values
+Album.pluck(:band_name).uniq
 
-# bad
-# an association on an instance will return a CollectionProxy
-instance.assoc.pluck(:id).uniq
+# bad - redundantly fetches duplicate values
+Album.where(year: 1985).pluck(:band_name).uniq
 
-# bad
-Model.pluck(:id).uniq
+# bad - redundantly fetches duplicate values
+customer.favourites.pluck(:color).uniq
 
 # good
-Model.distinct.pluck(:id)
+Album.distinct.pluck(:band_name)
+Album.distinct.where(year: 1985).pluck(:band_name)
+customer.favourites.distinct.pluck(:color)
 ----
 
 === Configurable attributes

lib/rubocop/cop/rails/uniq_before_pluck.rb

@@ -3,59 +3,56 @@
 module RuboCop
   module Cop
     module Rails
-      # Prefer the use of distinct, before pluck instead of after.
+      # Prefer using `distinct` before `pluck` instead of `uniq` after `pluck`.
       #
-      # The use of distinct before pluck is preferred because it executes within
+      # The use of distinct before pluck is preferred because it executes by
       # the database.
       #
       # This cop has two different enforcement modes. When the EnforcedStyle
-      # is conservative (the default) then only calls to pluck on a constant
-      # (i.e. a model class) before distinct are added as offenses.
+      # is `conservative` (the default), then only calls to `pluck` on a constant
+      # (i.e. a model class) before `uniq` are added as offenses.
       #
-      # When the EnforcedStyle is aggressive then all calls to pluck before
+      # When the EnforcedStyle is `aggressive` then all calls to `pluck` before
       # distinct are added as offenses. This may lead to false positives
-      # as the cop cannot distinguish between calls to pluck on an
+      # as the cop cannot distinguish between calls to `pluck` on an
       # ActiveRecord::Relation vs a call to pluck on an
       # ActiveRecord::Associations::CollectionProxy.
       #
       # @safety
-      #   This cop is unsafe because the behavior may change depending on the
-      #   database collation.
-      #   Autocorrect is disabled by default for this cop since it may generate
-      #   false positives.
+      #   This cop is unsafe for autocorrection because the behavior may change
+      #   depending on the database collation.
       #
       # @example EnforcedStyle: conservative (default)
-      #   # bad
-      #   Model.pluck(:id).uniq
+      #   # bad - redundantly fetches duplicate values
+      #   Album.pluck(:band_name).uniq
       #
       #   # good
-      #   Model.distinct.pluck(:id)
+      #   Album.distinct.pluck(:band_name)
       #
       # @example EnforcedStyle: aggressive
-      #   # bad
-      #   # this will return a Relation that pluck is called on
-      #   Model.where(cond: true).pluck(:id).uniq
+      #   # bad - redundantly fetches duplicate values
+      #   Album.pluck(:band_name).uniq
       #
-      #   # bad
-      #   # an association on an instance will return a CollectionProxy
-      #   instance.assoc.pluck(:id).uniq
+      #   # bad - redundantly fetches duplicate values
+      #   Album.where(year: 1985).pluck(:band_name).uniq
       #
-      #   # bad
-      #   Model.pluck(:id).uniq
+      #   # bad - redundantly fetches duplicate values
+      #   customer.favourites.pluck(:color).uniq
       #
       #   # good
-      #   Model.distinct.pluck(:id)
+      #   Album.distinct.pluck(:band_name)
+      #   Album.distinct.where(year: 1985).pluck(:band_name)
+      #   customer.favourites.distinct.pluck(:color)
       #
       class UniqBeforePluck < Base
         include ConfigurableEnforcedStyle
         include RangeHelp
         extend AutoCorrector
 
         MSG = 'Use `distinct` before `pluck`.'
-        RESTRICT_ON_SEND = %i[uniq distinct pluck].freeze
+        RESTRICT_ON_SEND = %i[uniq].freeze
         NEWLINE = "\n"
-        PATTERN = '[!^block (send (send %<type>s :pluck ...) ' \
-                  '${:uniq :distinct} ...)]'
+        PATTERN = '[!^block (send (send %<type>s :pluck ...) :uniq ...)]'
 
         def_node_matcher :conservative_node_match,
                          format(PATTERN, type: 'const')
@@ -64,13 +61,13 @@ class UniqBeforePluck < Base
                          format(PATTERN, type: '_')
 
         def on_send(node)
-          method = if style == :conservative
-                     conservative_node_match(node)
-                   else
-                     aggressive_node_match(node)
-                   end
+          uniq = if style == :conservative
+                   conservative_node_match(node)
+                 else
+                   aggressive_node_match(node)
+                 end
 
-          return unless method
+          return unless uniq
 
           add_offense(node.loc.selector) do |corrector|
             method = node.method_name
@@ -82,10 +79,6 @@ def on_send(node)
 
         private
 
-        def style_parameter_name
-          'EnforcedStyle'
-        end
-
         def dot_method_with_whitespace(method, node)
           range_between(dot_method_begin_pos(method, node),
                         node.loc.selector.end_pos)

spec/rubocop/cop/rails/uniq_before_pluck_spec.rb

@@ -1,92 +1,117 @@
 # frozen_string_literal: true
 
 RSpec.describe RuboCop::Cop::Rails::UniqBeforePluck, :config do
-  shared_examples_for 'UniqBeforePluck cop' \
-    do |method, source, action, corrected = nil|
-      if action == :correct
-        it "finds the use of #{method} after pluck in #{source}" do
-          offenses = inspect_source(source)
-          expect(offenses.first.message).to eq('Use `distinct` before `pluck`.')
-          corrected_source = corrected || 'Model.distinct.pluck(:id)'
-          expect(autocorrect_source(source)).to eq(corrected_source)
-        end
-      else
-        it "ignores pluck without errors in #{source}" do
-          expect_no_offenses(source)
-        end
-      end
+  shared_examples_for 'mode-independent behavior' do
+    it 'corrects' do
+      expect_offense(<<~RUBY)
+        Model.pluck(:name).uniq
+                           ^^^^ Use `distinct` before `pluck`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        Model.distinct.pluck(:name)
+      RUBY
     end
 
-  shared_examples_for 'mode independent behavior' do |method|
-    it_behaves_like 'UniqBeforePluck cop', method,
-                    "Model.pluck(:id).#{method}", :correct
+    it 'corrects hanging period' do
+      expect_offense(<<~RUBY)
+        Model.pluck(:name)
+          .uniq
+           ^^^^ Use `distinct` before `pluck`.
+      RUBY
 
-    it_behaves_like 'UniqBeforePluck cop', method,
-                    ['Model.pluck(:id)',
-                     "  .#{method}"].join("\n"), :correct
+      expect_correction(<<~RUBY)
+        Model.distinct.pluck(:name)
+      RUBY
+    end
 
-    it_behaves_like 'UniqBeforePluck cop', method,
-                    ['Model.pluck(:id).',
-                     "  #{method}"].join("\n"), :correct
+    it 'corrects trailing period' do
+      expect_offense(<<~RUBY)
+        Model.pluck(:name).
+          uniq
+          ^^^^ Use `distinct` before `pluck`.
+      RUBY
 
-    context "#{method} before pluck" do
-      it_behaves_like 'UniqBeforePluck cop', method,
-                      "Model.where(foo: 1).#{method}.pluck(:something)", :ignore
+      expect_correction(<<~RUBY)
+        Model.distinct.pluck(:name)
+      RUBY
     end
 
-    context "#{method} without a receiver" do
-      it_behaves_like 'UniqBeforePluck cop', method,
-                      "#{method}.something", :ignore
+    it 'ignores uniq before pluck' do
+      expect_no_offenses(<<~RUBY)
+        Model.where(foo: 1).uniq.pluck(:something)
+      RUBY
     end
 
-    context "#{method} without pluck" do
-      it_behaves_like 'UniqBeforePluck cop', method,
-                      "Model.#{method}", :ignore
+    it 'ignores uniq without a receiver' do
+      expect_no_offenses(<<~RUBY)
+        uniq.something
+      RUBY
     end
 
-    context "#{method} with a block" do
-      it_behaves_like 'UniqBeforePluck cop', method,
-                      "Model.where(foo: 1).pluck(:id).#{method} { |k| k[0] }",
-                      :ignore
+    it 'ignores uniq without pluck' do
+      expect_no_offenses(<<~RUBY)
+        Model.uniq
+      RUBY
     end
-  end
-
-  shared_examples_for 'mode dependent offenses' do |method, action|
-    it_behaves_like 'UniqBeforePluck cop', method,
-                    "Model.scope.pluck(:id).#{method}", action,
-                    'Model.scope.distinct.pluck(:id)'
 
-    it_behaves_like 'UniqBeforePluck cop', method,
-                    "instance.assoc.pluck(:id).#{method}", action,
-                    'instance.assoc.distinct.pluck(:id)'
+    it 'ignores uniq with a block' do
+      expect_no_offenses(<<~RUBY)
+        Model.where(foo: 1).pluck(:name).uniq { |k| k[0] }
+      RUBY
+    end
   end
 
   it 'registers an offense' do
     expect_offense(<<~RUBY)
-      Model.pluck(:id).uniq
-                       ^^^^ Use `distinct` before `pluck`.
+      Model.pluck(:name).uniq
+                         ^^^^ Use `distinct` before `pluck`.
     RUBY
   end
 
-  %w[uniq distinct].each do |method|
-    context 'when the enforced mode is conservative' do
-      let(:cop_config) do
-        { 'EnforcedStyle' => 'conservative', 'AutoCorrect' => true }
-      end
+  context 'when the enforced mode is conservative' do
+    let(:cop_config) { { 'EnforcedStyle' => 'conservative' } }
 
-      it_behaves_like 'mode independent behavior', method
+    it_behaves_like 'mode-independent behavior'
 
-      it_behaves_like 'mode dependent offenses', method, :ignore
+    it 'ignores model with a scope' do
+      expect_no_offenses(<<~RUBY)
+        Model.scope.pluck(:name).uniq
+      RUBY
     end
 
-    context 'when the enforced mode is aggressive' do
-      let(:cop_config) do
-        { 'EnforcedStyle' => 'aggressive', 'AutoCorrect' => true }
-      end
+    it 'ignores uniq on an association' do
+      expect_no_offenses(<<~RUBY)
+        instance.assoc.pluck(:name).uniq
+      RUBY
+    end
+  end
+
+  context 'when the enforced mode is aggressive' do
+    let(:cop_config) { { 'EnforcedStyle' => 'aggressive' } }
+
+    it_behaves_like 'mode-independent behavior'
+
+    it 'corrects model with a scope' do
+      expect_offense(<<~RUBY)
+        Model.scope.pluck(:name).uniq
+                                 ^^^^ Use `distinct` before `pluck`.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        Model.scope.distinct.pluck(:name)
+      RUBY
+    end
 
-      it_behaves_like 'mode independent behavior', method
+    it 'corrects uniq on an association' do
+      expect_offense(<<~RUBY)
+        instance.assoc.pluck(:name).uniq
+                                    ^^^^ Use `distinct` before `pluck`.
+      RUBY
 
-      it_behaves_like 'mode dependent offenses', method, :correct
+      expect_correction(<<~RUBY)
+        instance.assoc.distinct.pluck(:name)
+      RUBY
     end
   end
 end