diff --git a/CHANGELOG.md b/CHANGELOG.md index 445378d682..9f42ce7461 100644 --- 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 index ba77ecba9a..13b806ed44 100644 --- a/lib/rubocop/cop/rails/reversible_migration.rb +++ b/lib/rubocop/cop/rails/reversible_migration.rb @@ -129,17 +129,13 @@ class ReversibleMigration < Cop MSG = '%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 index bae2e99f51..5c491f82e6 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)