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

Fix UniqBeforePluck cop #135

Merged
merged 1 commit into from
Jun 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* [#238](https://github.com/rubocop-hq/rubocop-rails/issues/238): Fix auto correction for `Rails/IndexBy` when the `.to_h` invocation is separated in multiple lines. ([@diogoosorio][])
* [#248](https://github.com/rubocop-hq/rubocop-rails/pull/248): Fix a false positive for `Rails/SaveBang` when `update` is called on `ENV`. ([@eugeneius][])
* [#251](https://github.com/rubocop-hq/rubocop-rails/pull/251): Fix a false positive for `Rails/FilePath` when the result of `Rails.root.join` is interpolated at the end of a string. ([@eugeneius][])
* [#91](https://github.com/rubocop-hq/rubocop-rails/issues/91): Fix `Rails/UniqBeforePluck` to not recommend using `uniq` in `ActiveRecord::Relation`s anymore since it was deprecated in Rails 5.0. ([@santib][], [@ghiculescu][])

### Changes

Expand Down Expand Up @@ -193,3 +194,4 @@
[@tejasbubane]: https://github.com/tejasbubane
[@diogoosorio]: https://github.com/diogoosorio
[@tabuchi0919]: https://github.com/tabuchi0919
[@ghiculescu]: https://github.com/ghiculescu
2 changes: 1 addition & 1 deletion config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ Rails/UniqBeforePluck:
Description: 'Prefer the use of uniq or distinct before pluck.'
Enabled: true
VersionAdded: '0.40'
VersionChanged: '0.47'
VersionChanged: '2.6'
EnforcedStyle: conservative
SupportedStyles:
- conservative
Expand Down
27 changes: 14 additions & 13 deletions docs/modules/ROOT/pages/cops_rails.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -3243,22 +3243,23 @@ Time.at(timestamp).in_time_zone
| Yes
| Yes
| 0.40
| 0.47
| 2.6
|===

Prefer the use of uniq (or distinct), before pluck instead of after.
Prefer the use of distinct, before pluck instead of after.

The use of uniq before pluck is preferred because it executes within
The use of distinct before pluck is preferred because it executes within
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 uniq are added as offenses.
(i.e. a model class) before distinct are added as offenses.

When the EnforcedStyle is aggressive then all calls to pluck before
uniq are added as offenses. This may lead to false positives as the cop
cannot distinguish between calls to pluck on an ActiveRecord::Relation
vs a call to pluck on an ActiveRecord::Associations::CollectionProxy.
distinct are added as offenses. This may lead to false positives
as the cop cannot distinguish between calls to pluck on an
ActiveRecord::Relation vs a call to pluck on an
ActiveRecord::Associations::CollectionProxy.

Autocorrect is disabled by default for this cop since it may generate
false positives.
Expand All @@ -3270,10 +3271,10 @@ false positives.
[source,ruby]
----
# bad
Model.pluck(:id).uniq
Model.pluck(:id).distinct

# good
Model.uniq.pluck(:id)
Model.distinct.pluck(:id)
----

==== EnforcedStyle: aggressive
Expand All @@ -3282,17 +3283,17 @@ Model.uniq.pluck(:id)
----
# bad
# this will return a Relation that pluck is called on
Model.where(cond: true).pluck(:id).uniq
Model.where(cond: true).pluck(:id).distinct

# bad
# an association on an instance will return a CollectionProxy
instance.assoc.pluck(:id).uniq
instance.assoc.pluck(:id).distinct

# bad
Model.pluck(:id).uniq
Model.pluck(:id).distinct

# good
Model.uniq.pluck(:id)
Model.distinct.pluck(:id)
----

=== Configurable attributes
Expand Down
29 changes: 15 additions & 14 deletions lib/rubocop/cop/rails/uniq_before_pluck.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,50 +3,51 @@
module RuboCop
module Cop
module Rails
# Prefer the use of uniq (or distinct), before pluck instead of after.
# Prefer the use of distinct, before pluck instead of after.
#
# The use of uniq before pluck is preferred because it executes within
# The use of distinct before pluck is preferred because it executes within
# 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 uniq are added as offenses.
# (i.e. a model class) before distinct are added as offenses.
#
# When the EnforcedStyle is aggressive then all calls to pluck before
# uniq are added as offenses. This may lead to false positives as the cop
# cannot distinguish between calls to pluck on an ActiveRecord::Relation
# vs a call to pluck on an ActiveRecord::Associations::CollectionProxy.
# distinct are added as offenses. This may lead to false positives
# as the cop cannot distinguish between calls to pluck on an
# ActiveRecord::Relation vs a call to pluck on an
# ActiveRecord::Associations::CollectionProxy.
#
# Autocorrect is disabled by default for this cop since it may generate
# false positives.
#
# @example EnforcedStyle: conservative (default)
# # bad
# Model.pluck(:id).uniq
# Model.pluck(:id).distinct
#
# # good
# Model.uniq.pluck(:id)
# Model.distinct.pluck(:id)
#
# @example EnforcedStyle: aggressive
# # bad
# # this will return a Relation that pluck is called on
# Model.where(cond: true).pluck(:id).uniq
# Model.where(cond: true).pluck(:id).distinct
#
# # bad
# # an association on an instance will return a CollectionProxy
# instance.assoc.pluck(:id).uniq
# instance.assoc.pluck(:id).distinct
#
# # bad
# Model.pluck(:id).uniq
# Model.pluck(:id).distinct
#
# # good
# Model.uniq.pluck(:id)
# Model.distinct.pluck(:id)
#
class UniqBeforePluck < RuboCop::Cop::Cop
include ConfigurableEnforcedStyle
include RangeHelp

MSG = 'Use `%<method>s` before `pluck`.'
MSG = 'Use `distinct` before `pluck`.'
NEWLINE = "\n"
PATTERN = '[!^block (send (send %<type>s :pluck ...) ' \
'${:uniq :distinct} ...)]'
Expand Down Expand Up @@ -75,7 +76,7 @@ def autocorrect(node)
method = node.method_name

corrector.remove(dot_method_with_whitespace(method, node))
corrector.insert_before(node.receiver.loc.dot.begin, ".#{method}")
corrector.insert_before(node.receiver.loc.dot.begin, '.distinct')
end
end

Expand Down
8 changes: 4 additions & 4 deletions spec/rubocop/cop/rails/uniq_before_pluck_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
if action == :correct
it "finds the use of #{method} after pluck in #{source}" do
inspect_source(source)
expect(cop.messages).to eq(["Use `#{method}` before `pluck`."])
expect(cop.messages).to eq(['Use `distinct` before `pluck`.'])
expect(cop.highlights).to eq([method])
corrected_source = corrected || "Model.#{method}.pluck(:id)"
corrected_source = corrected || 'Model.distinct.pluck(:id)'
expect(autocorrect_source(source)).to eq(corrected_source)
end
else
Expand Down Expand Up @@ -57,11 +57,11 @@
shared_examples_for 'mode dependent offenses' do |method, action|
it_behaves_like 'UniqBeforePluck cop', method,
"Model.scope.pluck(:id).#{method}", action,
"Model.scope.#{method}.pluck(:id)"
'Model.scope.distinct.pluck(:id)'

it_behaves_like 'UniqBeforePluck cop', method,
"instance.assoc.pluck(:id).#{method}", action,
"instance.assoc.#{method}.pluck(:id)"
'instance.assoc.distinct.pluck(:id)'
end

%w[uniq distinct].each do |method|
Expand Down