[Fix rubocop#144] false positive for Rails/ReversibleMigration

change_table_comment is only irreversible when passing a string as the comment. If we pass a hash with `:from` and `:to` keys, it works as a reversible migration Fixes rubocop#144
DNA · Nov 4, 2019 · 19e201a · 19e201a
1 parent 9a6058f
commit 19e201a
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,8 @@
 * [#131](https://github.com/rubocop-hq/rubocop-rails/pull/131): Fix an incorrect autocorrect for `Rails/Presence` when using `[]` method. ([@forresty][])
 * [#142](https://github.com/rubocop-hq/rubocop-rails/pull/142): Fix an incorrect autocorrect for `Rails/EnumHash` when using nested constants. ([@koic][])
 * [#136](https://github.com/rubocop-hq/rubocop-rails/pull/136): Fix a false positive for `Rails/ReversibleMigration` when using `change_default` with `:from` and `:to` options. ([@sinsoku][])
+* [#144](https://github.com/rubocop-hq/rubocop-rails/issues/144): Fix a false positive for `Rails/ReversibleMigration` when using change_table_comment with a `:from` and `:to` hash. ([@DNA][])
+
 
 ## 2.3.2 (2019-09-01)
 
@@ -97,3 +99,4 @@
 [@forresty]: https://github.com/forresty
 [@sinsoku]: https://github.com/sinsoku
 [@pocke]: https://github.com/pocke
+[@DNA]: https://github.com/DNA
diff --git a/lib/rubocop/cop/rails/reversible_migration.rb b/lib/rubocop/cop/rails/reversible_migration.rb
@@ -129,17 +129,13 @@ class ReversibleMigration < Cop
         MSG = '%<action>s is not reversible.'
 
         def_node_matcher :irreversible_schema_statement_call, <<-PATTERN
-          (send nil? ${:change_table_comment :execute :remove_belongs_to} ...)
+          (send nil? ${:execute :remove_belongs_to} ...)
         PATTERN
 
         def_node_matcher :drop_table_call, <<-PATTERN
           (send nil? :drop_table ...)
         PATTERN
 
-        def_node_matcher :change_column_default_call, <<-PATTERN
-          (send nil? :change_column_default {[(sym _) (sym _)] (splat _)} $...)
-        PATTERN
-
         def_node_matcher :remove_column_call, <<-PATTERN
           (send nil? :remove_column $...)
         PATTERN
@@ -158,7 +154,7 @@ def on_send(node)
 
           check_irreversible_schema_statement_node(node)
           check_drop_table_node(node)
-          check_change_column_default_node(node)
+          check_reversible_hash_node(node)
           check_remove_column_node(node)
           check_remove_foreign_key_node(node)
         end
@@ -190,17 +186,15 @@ def check_drop_table_node(node)
           end
         end
 
-        def check_change_column_default_node(node)
-          change_column_default_call(node) do |args|
-            unless all_hash_key?(args.last, :from, :to)
-              add_offense(
-                node,
-                message: format(
-                  MSG, action: 'change_column_default(without :from and :to)'
-                )
-              )
-            end
-          end
+        def check_reversible_hash_node(node)
+          return if reversible_change_table_call?(node)
+
+          add_offense(
+            node,
+            message: format(
+              MSG, action: "#{node.method_name}(without :from and :to)"
+            )
+          )
         end
 
         def check_remove_column_node(node)
@@ -253,7 +247,8 @@ def reversible_change_table_call?(node)
           case node.method_name
           when :change, :remove
             false
-          when :change_default
+          when :change_default, :change_column_default, :change_table_comment,
+               :change_column_comment
             all_hash_key?(node.arguments.last, :from, :to)
           else
             true

diff --git a/spec/rubocop/cop/rails/reversible_migration_spec.rb b/spec/rubocop/cop/rails/reversible_migration_spec.rb
@@ -126,6 +126,30 @@ def change
     RUBY
   end
 
+  context 'change_table_comment' do
+    it_behaves_like 'accepts',
+                    'change_table_comment(with :from and :to)', <<-RUBY
+      change_table_comment(:posts, from: nil, to: "draft")
+    RUBY
+
+    it_behaves_like 'offense',
+                    'change_table_comment(without :from and :to)', <<-RUBY
+      change_table_comment(:suppliers, 'new')
+    RUBY
+  end
+
+  context 'change_column_comment' do
+    it_behaves_like 'accepts',
+                    'change_column_comment(with :from and :to)', <<-RUBY
+      change_column_comment(:posts, :state, from: nil, to: "draft")
+    RUBY
+
+    it_behaves_like 'offense',
+                    'change_column_comment(without :from and :to)', <<-RUBY
+      change_column_comment(:suppliers, :qualification, 'new')
+    RUBY
+  end
+
   context 'remove_column' do
     it_behaves_like 'accepts', 'remove_column(with type)', <<-RUBY
       remove_column(:suppliers, :qualification, :string)