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

[BUG] Use uniq before pluck #3148

Closed
kvokka opened this issue May 19, 2016 · 6 comments
Closed

[BUG] Use uniq before pluck #3148

kvokka opened this issue May 19, 2016 · 6 comments

Comments

@kvokka
Copy link

kvokka commented May 19, 2016

In model i have

  def events_ids
    result = volunteer_roles.uniq.pluck(:event_id)
    result.any? ? result : nil
  end

rubocop -a returns

Offenses:

app/models/user.rb:157:47: C: [Corrected] Use uniq before pluck
    result = volunteer_roles.pluck(:event_id).uniq
                                              ^^^^

And in this case, it is wrong correction, because uniq or distinct methods return Array and pluck need Activerecord::Relation

@alexdowad
Copy link
Contributor

@tjwp?

@kvokka
Copy link
Author

kvokka commented May 30, 2016

I cannot replay it now, but as I see it is closed 👍

@kvokka kvokka closed this as completed May 30, 2016
@IngoAlbers
Copy link

This is not fixed and should be reopened.

See: #3122 (comment)

@jonas054
Copy link
Collaborator

jonas054 commented Jun 3, 2016

I agree. It's not fixed.

@jonas054 jonas054 reopened this Jun 3, 2016
@tjwp
Copy link
Contributor

tjwp commented Jun 6, 2016

I don't think we can easily distinguish between uniq being called on CollectionProxy vs an ActiveRecord::Relation :(

I should either:

  1. Make this cop disabled by default, remove autocorrection, and comment on when it may give a false positive. The cop has been useful for us, and may be helpful for others that do not typically use uniq or distinct on associations.
  2. Remove the cop. I expect we'll continue to use it, but I'll just move it to a custom cop in our code.

Let me know what is preferred in situations like this.

dzaporozhets pushed a commit to gitlabhq/gitlabhq that referenced this issue Jun 8, 2016
Disable Rails/UniqBeforePluck rubocop cop

 Rails/UniqBeforePluck seems to have some bugs 

* rubocop/rubocop#3122
* rubocop/rubocop#3148

and we had some problems in EE with that https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/425#note_12245005

See merge request !4477
@jonas054
Copy link
Collaborator

Maybe we can still "save" this cop by making it more conservative. I mean, we could start with some simple expressions that we believe will not generate many false positives. For example:

MyModel.pluck(:col).uniq

where the receiver looks like a class. We let the cop only report such expressions.

But even so, removing (or disabling) auto-correct might be necessary.

Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016
…cop#3231)

Autocorrect is disabled by default for this cop since it can
potentially generate false positives.

Add offenses for `distinct` as well as `uniq`.

By default only add offenses for calls to `pluck` directly
on constants.

Add an 'aggressive' mode that adds all offenses as the cop did
originally.
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

5 participants