rubocop-rails icon indicating copy to clipboard operation
rubocop-rails copied to clipboard

Add cop for checking duplicate assingments to `ignored_columns`

Open fsateler opened this issue 3 years ago • 9 comments

Overwriting previous assignments is usually a mistake, since it will un-ignore the first set of columns


Before submitting the PR make sure the following are checked:

  • [x] Wrote good commit messages.
  • [x] Commit message starts with [Fix #issue-number] (if the related issue exists).
  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [x] Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • [x] Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
  • [] If this is a new cop, consider making a corresponding update to the Rails Style Guide. (I don't think this is needed)

fsateler avatar Jun 30 '21 20:06 fsateler

I think the following should be technically permitted.

expect_no_offenses(<<~RUBY)
  class User < ActiveRecord::Base
    if condition
      self.ignored_columns = [:one]
    else
      self.ignored_columns = [:two]
    end
  end
RUBY

koic avatar Jul 01 '21 11:07 koic

I think the following should be technically permitted.

Oh, good point. Indeed it should. Any hints on how to approach that? A cop that does something similar to ~copy~ get inspiration?

fsateler avatar Jul 01 '21 17:07 fsateler

I'm not sure, but Style/IdenticalConditionalBranches may be a hint to check that different branches have the same statement. https://github.com/rubocop/rubocop/blob/master/lib/rubocop/cop/style/identical_conditional_branches.rb

koic avatar Jul 05 '21 18:07 koic

I couldn't make it work with conditional branches :(. Will that be a blocker?

fsateler avatar Dec 31 '21 13:12 fsateler

I added a PR for rails style guide https://github.com/rubocop/rails-style-guide/pull/301

fsateler avatar Jan 05 '22 18:01 fsateler

@fsateler @koic @pirj Waiting for this PR to be merged. What's Blocker? (Though I think https://github.com/rubocop/rubocop-rails/pull/514#issuecomment-1003382329 is related)

kkitadate avatar Sep 12 '22 01:09 kkitadate

Can you please force-push to trigger CI, @kkitadate ?

pirj avatar Sep 12 '22 06:09 pirj

Can you please force-push to trigger CI, @kkitadate ?

Sure. I will open another PR as I can't push to this branch.

kkitadate avatar Sep 12 '22 06:09 kkitadate

@pirj I opened https://github.com/rubocop/rubocop-rails/pull/771. It also seems to have passed CI.

kkitadate avatar Sep 12 '22 07:09 kkitadate

@fsateler I'm going close this PR because #771 has been merged. I'm sorry about should have proceeded this one. So, I've added your account name to the changelog entry as the original author. https://github.com/rubocop/rubocop-rails/commit/7a434e46f8be3c1c21d13a15a3c845572ace2b72

Thank you.

koic avatar Oct 20 '22 01:10 koic

@koic thanks for the kind words. I'm glad someone else made it work faster. As long as it happened I'm happy :)

Thank you for maintaining rubocop-rails!

fsateler avatar Oct 20 '22 03:10 fsateler