From 7343d57c676f5f9b1e956ee7d09b4d70635c132a Mon Sep 17 00:00:00 2001 From: Leonardo Prado Date: Thu, 31 Oct 2019 16:47:16 -0300 Subject: [PATCH] [Fix #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 #144 --- CHANGELOG.md | 3 ++ lib/rubocop/cop/rails/reversible_migration.rb | 38 ++++++++++++++++++- .../cop/rails/reversible_migration_spec.rb | 24 ++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ecb6037ef..4837d9783b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ * [#120](https://github.com/rubocop-hq/rubocop-rails/issues/120): Fix message for `Rails/SaveBang` when the save is in the body of a conditional. ([@jas14][]) * [#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][]) +* [#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) @@ -93,3 +95,4 @@ [@eugeneius]: https://github.com/eugeneius [@jas14]: https://github.com/jas14 [@forresty]: https://github.com/forresty +[@DNA]: https://github.com/DNA diff --git a/lib/rubocop/cop/rails/reversible_migration.rb b/lib/rubocop/cop/rails/reversible_migration.rb index 3484c6b59d..f608b44d47 100644 --- a/lib/rubocop/cop/rails/reversible_migration.rb +++ b/lib/rubocop/cop/rails/reversible_migration.rb @@ -132,7 +132,7 @@ class ReversibleMigration < Cop ].freeze 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 @@ -143,6 +143,14 @@ class ReversibleMigration < Cop (send nil? :change_column_default {[(sym _) (sym _)] (splat _)} $...) PATTERN + def_node_matcher :change_table_comment_call, <<-PATTERN + (send nil? :change_table_comment {[(sym _)]} $...) + PATTERN + + def_node_matcher :change_column_comment_call, <<-PATTERN + (send nil? :change_column_comment {[(sym _)]} $...) + PATTERN + def_node_matcher :remove_column_call, <<-PATTERN (send nil? :remove_column $...) PATTERN @@ -162,6 +170,8 @@ def on_send(node) check_irreversible_schema_statement_node(node) check_drop_table_node(node) check_change_column_default_node(node) + check_change_table_comment_node(node) + check_change_column_comment_node(node) check_remove_column_node(node) check_remove_foreign_key_node(node) end @@ -206,6 +216,32 @@ def check_change_column_default_node(node) end end + def check_change_table_comment_node(node) + change_table_comment_call(node) do |args| + unless all_hash_key?(args.last, :from, :to) + add_offense( + node, + message: format( + MSG, action: 'change_table_comment(without :from and :to)' + ) + ) + end + end + end + + def check_change_column_comment_node(node) + change_column_comment_call(node) do |args| + unless all_hash_key?(args.last, :from, :to) + add_offense( + node, + message: format( + MSG, action: 'change_column_comment(without :from and :to)' + ) + ) + end + end + end + def check_remove_column_node(node) remove_column_call(node) do |args| if args.to_a.size < 3 diff --git a/spec/rubocop/cop/rails/reversible_migration_spec.rb b/spec/rubocop/cop/rails/reversible_migration_spec.rb index 45dfb9dcde..ec87a42cbe 100644 --- 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)