Skip to content

Commit 816478b

Browse files
fsatelerkkitadate
authored andcommitted
Add cop for checking assingments to ignored_columns
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.
1 parent 181b497 commit 816478b

File tree

5 files changed

+121
-0
lines changed

5 files changed

+121
-0
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#771](https://github.com/rubocop/rubocop-rails/pull/771): Add new `Rails/IgnoredColumnsAssignment` cop. ([@kkitadate][])

config/default.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,6 +537,13 @@ Rails/I18nLocaleTexts:
537537
Enabled: pending
538538
VersionAdded: '2.14'
539539

540+
Rails/IgnoredColumnsAssignment:
541+
Description: 'Looks for assignments of `ignored_columns` that override previous assignments.'
542+
StyleGuide: 'https://rails.rubystyle.guide/#append-ignored-columns'
543+
Enabled: pending
544+
SafeAutoCorrect: false
545+
VersionAdded: '<<next>>'
546+
540547
Rails/IgnoredSkipActionFilterOption:
541548
Description: 'Checks that `if` and `only` (or `except`) are not used together as options of `skip_*` action filter.'
542549
Reference: 'https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options'
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# This cops looks for assignments of `ignored_columns` that may override previous
7+
# assignments.
8+
#
9+
# Overwriting previous assignments is usually a mistake, since it will
10+
# un-ignore the first set of columns. Since duplicate column names is not
11+
# a problem, it is better to simply append to the list.
12+
#
13+
# @example
14+
#
15+
# # bad
16+
# class User < ActiveRecord::Base
17+
# self.ignored_columns = [:one]
18+
# end
19+
#
20+
# # bad
21+
# class User < ActiveRecord::Base
22+
# self.ignored_columns = [:one, :two]
23+
# end
24+
#
25+
# # good
26+
# class User < ActiveRecord::Base
27+
# self.ignored_columns += [:one, :two]
28+
# end
29+
#
30+
# # good
31+
# class User < ActiveRecord::Base
32+
# self.ignored_columns += [:one]
33+
# self.ignored_columns += [:two]
34+
# end
35+
#
36+
class IgnoredColumnsAssignment < Base
37+
extend AutoCorrector
38+
39+
MSG = 'Use `+=` instead of `=`.'
40+
RESTRICT_ON_SEND = %i[ignored_columns=].freeze
41+
42+
def on_send(node)
43+
add_offense(node.loc.operator) do |corrector|
44+
corrector.replace(node.loc.operator, '+=')
45+
end
46+
end
47+
end
48+
end
49+
end
50+
end

lib/rubocop/cop/rails_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
require_relative 'rails/i18n_lazy_lookup'
6262
require_relative 'rails/i18n_locale_assignment'
6363
require_relative 'rails/i18n_locale_texts'
64+
require_relative 'rails/ignored_columns_assignment'
6465
require_relative 'rails/ignored_skip_action_filter_option'
6566
require_relative 'rails/index_by'
6667
require_relative 'rails/index_with'
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Rails::IgnoredColumnsAssignment, :config do
4+
it 'registers an offense when using `ignored_columns` once' do
5+
expect_offense(<<~RUBY)
6+
class User < ActiveRecord::Base
7+
self.ignored_columns = [:one]
8+
^ Use `+=` instead of `=`.
9+
end
10+
RUBY
11+
12+
expect_correction(<<~RUBY)
13+
class User < ActiveRecord::Base
14+
self.ignored_columns += [:one]
15+
end
16+
RUBY
17+
end
18+
19+
it 'registers an offense when setting `ignored_columns` twice' do
20+
expect_offense(<<~RUBY)
21+
class User < ActiveRecord::Base
22+
self.ignored_columns = [:one]
23+
^ Use `+=` instead of `=`.
24+
self.ignored_columns = [:two]
25+
^ Use `+=` instead of `=`.
26+
end
27+
RUBY
28+
29+
expect_correction(<<~RUBY)
30+
class User < ActiveRecord::Base
31+
self.ignored_columns += [:one]
32+
self.ignored_columns += [:two]
33+
end
34+
RUBY
35+
end
36+
37+
it 'registers an offense when setting `ignored_columns` after appending' do
38+
expect_offense(<<~RUBY)
39+
class User < ActiveRecord::Base
40+
self.ignored_columns += [:one]
41+
self.ignored_columns = [:two]
42+
^ Use `+=` instead of `=`.
43+
end
44+
RUBY
45+
46+
expect_correction(<<~RUBY)
47+
class User < ActiveRecord::Base
48+
self.ignored_columns += [:one]
49+
self.ignored_columns += [:two]
50+
end
51+
RUBY
52+
end
53+
54+
it 'does not register an offense when appending to `ignored_columns` and then appending' do
55+
expect_no_offenses(<<~RUBY)
56+
class User < ActiveRecord::Base
57+
self.ignored_columns += [:one]
58+
self.ignored_columns += [:two]
59+
end
60+
RUBY
61+
end
62+
end

0 commit comments

Comments
 (0)