Add cop for checking assingments to `ignored_columns` #771

kkitadate · 2022-09-12T07:29:35Z

This PR is a rebase of #514 on rubocop:master.
It is requested by #514 (comment).

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
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.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
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.
If this is a new cop, consider making a corresponding update to the Rails Style Guide.

koic · 2022-09-12T07:57:43Z

The issue to be resolved conflicts with #761. We might need some time to figure out which one is better.
Anyway, the next release is a bug fix, so we have time.
cc @sikachu

sikachu · 2022-09-12T08:10:20Z

Oh, interesting. So this approach is to suggest that we always do += instead of = altogether.

I think this approach definitely makes the cop super clean, as you just detect for = instead of +=.

However, I find that it is a bit weird since if you look at just the model itself, you'll have to use += even for the first call. You have to have background knowledge of "Rails sets this variable to [], so that's why we can just use +=.

In our codebase, we never use += but we did have a few accidents of calling = a few times. Hence, I'm in favor of the = version since it's more clear without the need of the background knowledge.

pirj · 2022-09-12T09:23:42Z

figure out which one is better

I'd go with +=, keeping in mind inheritance and mixins.

fsateler · 2022-09-26T19:14:59Z

For reference, #761 also started as a cop checking for =. However, due to the impossibility of checking for mixins and inheritance, this cop might not flag invalid behavior, while it is always correct to use +=.

sikachu · 2022-09-27T06:11:50Z

Yeah, I'm retracting my #761. Let's use += since it works better for inheritance and it makes it much easier to auto-fix it. 👍

koic · 2022-09-27T18:14:37Z

Thank you for the opinions! Let us move on with this cop.

koic · 2022-09-27T18:14:40Z

config/default.yml

@@ -899,6 +899,12 @@ Rails/ScopeArgs:
  Include:
    - app/models/**/*.rb

+Rails/SetIgnoredColumns:
+  Description: 'Looks for assignments of `ignored_columns` that override previous assignments.'


Can you add StyleGuide URL?

Suggested change

Description: 'Looks for assignments of `ignored_columns` that override previous assignments.'

Description: 'Looks for assignments of `ignored_columns` that override previous assignments.'

StyleGuide: 'https://rails.rubystyle.guide/#append-ignored-columns'

koic · 2022-09-27T18:14:44Z

lib/rubocop/cop/rails/set_ignored_columns.rb

+      #     self.ignored_columns += [:two]
+      #   end
+      #
+      class SetIgnoredColumns < Base


I'm not sure if a name starting with Set is better for this case.
How about cop name like Rails/AppendIgnoredColumns or Rails/IgnoredColumnsAssignment?

Suggested change

class SetIgnoredColumns < Base

class AppendIgnoredColumns < Base

I prefer Rails/IgnoredColumnsAssignment because it looks like a noun.

koic · 2022-09-27T18:14:46Z

spec/rubocop/cop/rails/set_ignored_columns_spec.rb

+        self.ignored_columns = [:one]
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This assignment to `ignored_columns` may overwrite previous ones.


Can you tweak the offense range and message?

Suggested change

self.ignored_columns = [:one]

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This assignment to `ignored_columns` may overwrite previous ones.

self.ignored_columns = [:one]

^ Use `+=` instead of `=`.

kkitadate · 2022-09-29T10:22:35Z

CI seems to have failed because rubocop/rubocop#11002 was merged 5 hours ago.
#798 will fix this failure.

koic · 2022-10-01T03:19:54Z

lib/rubocop/cop/rails/ignored_columns_assignment.rb

+        extend AutoCorrector
+        MSG = 'Use `+=` instead of `=`.'
+
+        RESTRICT_ON_SEND = %i[ignored_columns=].freeze


That's just my two cents. Can you tweak it?

Suggested change

extend AutoCorrector

MSG = 'Use `+=` instead of `=`.'

RESTRICT_ON_SEND = %i[ignored_columns=].freeze

extend AutoCorrector

MSG = 'Use `+=` instead of `=`.'

RESTRICT_ON_SEND = %i[ignored_columns=].freeze

kkitadate · 2022-10-01T05:08:13Z

lib/rubocop/cop/rails_cops.rb

@@ -107,6 +107,7 @@
 require_relative 'rails/save_bang'
 require_relative 'rails/schema_comment'
 require_relative 'rails/scope_args'
+require_relative 'rails/ignored_columns_assignment'


The line that adds this new Cop is wrong so I will also fix it.

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.

koic · 2022-10-02T08:01:28Z

Thanks!

koic · 2022-10-20T01:20:03Z

changelog/new_ignored_columns_assignment.md

@@ -0,0 +1 @@
+* [#771](https://github.com/rubocop/rubocop-rails/pull/771): Add new `Rails/IgnoredColumnsAssignment` cop. ([@kkitadate][])


JFYI, this changelog entry has been updated in the edge because #514 (comment).

kkitadate mentioned this pull request Sep 12, 2022

Add cop for checking duplicate assingments to ignored_columns #514

Closed

8 tasks

sikachu mentioned this pull request Sep 27, 2022

Add new Rails/DuplicateIgnoredColumns cop #761

Closed

9 tasks

koic reviewed Sep 27, 2022

View reviewed changes

kkitadate force-pushed the duplicate-ignored-columns branch from d98e857 to 2723be4 Compare September 29, 2022 09:38

kkitadate force-pushed the duplicate-ignored-columns branch 2 times, most recently from 228932c to 4f415c8 Compare September 30, 2022 01:16

kkitadate requested a review from koic September 30, 2022 22:53

koic reviewed Oct 1, 2022

View reviewed changes

kkitadate commented Oct 1, 2022

View reviewed changes

kkitadate force-pushed the duplicate-ignored-columns branch from 4f415c8 to 816478b Compare October 1, 2022 05:09

kkitadate requested a review from koic October 1, 2022 05:14

koic merged commit 43673c1 into rubocop:master Oct 2, 2022

koic reviewed Oct 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cop for checking assingments to `ignored_columns` #771

Add cop for checking assingments to `ignored_columns` #771

kkitadate commented Sep 12, 2022

koic commented Sep 12, 2022

sikachu commented Sep 12, 2022

pirj commented Sep 12, 2022

fsateler commented Sep 26, 2022

sikachu commented Sep 27, 2022

koic commented Sep 27, 2022

koic Sep 27, 2022

koic Sep 27, 2022

kkitadate Sep 29, 2022

koic Sep 27, 2022

kkitadate commented Sep 29, 2022 •

edited

Loading

koic Oct 1, 2022

kkitadate Oct 1, 2022 •

edited

Loading

koic commented Oct 2, 2022

koic Oct 20, 2022

	Description: 'Looks for assignments of `ignored_columns` that override previous assignments.'
	Description: 'Looks for assignments of `ignored_columns` that override previous assignments.'
	StyleGuide: 'https://rails.rubystyle.guide/#append-ignored-columns'

	class SetIgnoredColumns < Base
	class AppendIgnoredColumns < Base

		self.ignored_columns = [:one]
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This assignment to `ignored_columns` may overwrite previous ones.

		@@ -0,0 +1 @@
		* [#771](https://github.com/rubocop/rubocop-rails/pull/771): Add new `Rails/IgnoredColumnsAssignment` cop. ([@kkitadate][])

Add cop for checking assingments to ignored_columns #771

Add cop for checking assingments to ignored_columns #771

Conversation

kkitadate commented Sep 12, 2022

koic commented Sep 12, 2022

sikachu commented Sep 12, 2022

pirj commented Sep 12, 2022

fsateler commented Sep 26, 2022

sikachu commented Sep 27, 2022

koic commented Sep 27, 2022

koic Sep 27, 2022

Choose a reason for hiding this comment

koic Sep 27, 2022

Choose a reason for hiding this comment

kkitadate Sep 29, 2022

Choose a reason for hiding this comment

koic Sep 27, 2022

Choose a reason for hiding this comment

kkitadate commented Sep 29, 2022 • edited Loading

koic Oct 1, 2022

Choose a reason for hiding this comment

kkitadate Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

koic commented Oct 2, 2022

koic Oct 20, 2022

Choose a reason for hiding this comment

Add cop for checking assingments to `ignored_columns` #771

Add cop for checking assingments to `ignored_columns` #771

kkitadate commented Sep 29, 2022 •

edited

Loading

kkitadate Oct 1, 2022 •

edited

Loading