Merge pull request #635 from eugeneius/bulk_change_table_remove_multiple_columns

Handle t.remove with multiple columns in Rails/BulkChangeTable

Author: koic

changelog/fix_bulk_change_table_remove_multiple_columns.md

@@ -0,0 +1 @@
+* [#635](https://github.com/rubocop/rubocop-rails/pull/635): Handle `t.remove` with multiple columns in `Rails/BulkChangeTable`. ([@eugeneius][])

lib/rubocop/cop/rails/bulk_change_table.rb

@@ -155,17 +155,31 @@ def on_send(node)
           return if include_bulk_options?(node)
           return unless node.block_node
 
-          send_nodes = node.block_node.body.each_child_node(:send).to_a
+          send_nodes = send_nodes_from_change_table_block(node.block_node.body)
 
-          transformations = send_nodes.select do |send_node|
-            combinable_transformations.include?(send_node.method_name)
-          end
-
-          add_offense_for_change_table(node) if transformations.size > 1
+          add_offense_for_change_table(node) if count_transformations(send_nodes) > 1
         end
 
         private
 
+        def send_nodes_from_change_table_block(body)
+          if body.send_type?
+            [body]
+          else
+            body.each_child_node(:send).to_a
+          end
+        end
+
+        def count_transformations(send_nodes)
+          send_nodes.sum do |node|
+            if node.method?(:remove)
+              node.arguments.count { |arg| !arg.hash_type? }
+            else
+              combinable_transformations.include?(node.method_name) ? 1 : 0
+            end
+          end
+        end
+
         # @param node [RuboCop::AST::SendNode] (send nil? :change_table ...)
         def include_bulk_options?(node)
           # arguments: [{(sym :table)(str "table")} (hash (pair (sym :bulk) _))]

spec/rubocop/cop/rails/bulk_change_table_spec.rb

@@ -387,6 +387,48 @@ def change
         end
       RUBY
     end
+
+    it 'registers an offense for a single `t.remove` with multiple columns' do
+      expect_offense(<<~RUBY)
+        def change
+          change_table :users do |t|
+          ^^^^^^^^^^^^^^^^^^^ You can combine alter queries using `bulk: true` options.
+            t.remove :name, :metadata
+          end
+        end
+      RUBY
+    end
+
+    it 'does not register an offense for a single `t.remove` with one column' do
+      expect_no_offenses(<<~RUBY)
+        def change
+          change_table :users do |t|
+            t.remove :name
+          end
+        end
+      RUBY
+    end
+
+    it 'registers an offense for a single `t.remove` with multiple columns and options' do
+      expect_offense(<<~RUBY)
+        def change
+          change_table :users do |t|
+          ^^^^^^^^^^^^^^^^^^^ You can combine alter queries using `bulk: true` options.
+            t.remove :name, :metadata, type: :string
+          end
+        end
+      RUBY
+    end
+
+    it 'does not register an offense for a single `t.remove` with one column and options' do
+      expect_no_offenses(<<~RUBY)
+        def change
+          change_table :users do |t|
+            t.remove :name, type: :string
+          end
+        end
+      RUBY
+    end
   end
 
   context 'when database is PostgreSQL' do