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

Add cop for checking duplicate assingments to ignored_columns #514

Closed
wants to merge 1 commit into from

Conversation

fsateler
Copy link
Contributor

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:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • 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.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • 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)

@koic
Copy link
Member

koic commented Jul 1, 2021

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

@fsateler
Copy link
Contributor Author

fsateler commented Jul 1, 2021

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?

@koic
Copy link
Member

koic commented Jul 5, 2021

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

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I believe we can make it stricter with no negative impact.

config/default.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/cops.adoc Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/duplicate_ignored_columns.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/duplicate_ignored_columns.rb Outdated Show resolved Hide resolved
@fsateler
Copy link
Contributor Author

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

Setting `ignored_columns` may overwrite previous assignments, and that
is problematic because it can un-ignore a previously set column list.
Since it is not a problem to have a duplicate column in the list,
simply recommend always appending.
fsateler added a commit to fsateler/rails-style-guide that referenced this pull request Jan 5, 2022
Setting may overwrite previous assignments and that is almost always a mistake.

See also rubocop/rubocop-rails#514
@fsateler
Copy link
Contributor Author

fsateler commented Jan 5, 2022

I added a PR for rails style guide rubocop/rails-style-guide#301

pirj pushed a commit to fsateler/rails-style-guide that referenced this pull request May 5, 2022
Setting may overwrite previous assignments and that is almost always a mistake.

See also rubocop/rubocop-rails#514
pirj pushed a commit to fsateler/rails-style-guide that referenced this pull request May 5, 2022
Setting may overwrite previous assignments and that is almost always a mistake.

See also rubocop/rubocop-rails#514
pirj pushed a commit to rubocop/rails-style-guide that referenced this pull request May 5, 2022
Setting may overwrite previous assignments and that is almost always a mistake.

See also rubocop/rubocop-rails#514
@kkitadate
Copy link
Contributor

@fsateler @koic @pirj Waiting for this PR to be merged. What's Blocker? (Though I think #514 (comment) is related)

@pirj
Copy link
Member

pirj commented Sep 12, 2022

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

@kkitadate
Copy link
Contributor

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

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

@kkitadate
Copy link
Contributor

kkitadate commented Sep 12, 2022

@pirj I opened #771. It also seems to have passed CI.

koic added a commit that referenced this pull request Oct 20, 2022
@koic
Copy link
Member

koic commented Oct 20, 2022

@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.
7a434e4

Thank you.

@koic koic closed this Oct 20, 2022
@fsateler
Copy link
Contributor Author

@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 fsateler deleted the duplicate-ignored-columns branch October 20, 2022 03:06
goldapple911 added a commit to goldapple911/rails-style-guide that referenced this pull request Aug 15, 2023
Setting may overwrite previous assignments and that is almost always a mistake.

See also rubocop/rubocop-rails#514
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

Successfully merging this pull request may close these issues.

4 participants