checks.rb in strong_migrations-1.1.0

- old
+ new

@@ -1,22 +1,22 @@
 # TODO better pattern
 module StrongMigrations
   module Checks
     private
 
-    def check_add_check_constraint(args)
-      table, expression, options = args
-      options ||= {}
+    def check_add_check_constraint(*args)
+      options = args.extract_options!
+      table, expression = args
 
       if !new_table?(table)
         if postgresql? && options[:validate] != false
           add_options = options.merge(validate: false)
           name = options[:name] || @migration.check_constraint_options(table, expression, options)[:name]
           validate_options = {name: name}
 
           if StrongMigrations.safe_by_default
-            safe_add_check_constraint(table, expression, add_options, validate_options)
+            safe_add_check_constraint(*args, add_options, validate_options)
             throw :safe
           end
 
           raise_error :add_check_constraint,
             add_check_constraint_code: command_str("add_check_constraint", [table, expression, add_options]),
@@ -25,13 +25,13 @@
           raise_error :add_check_constraint_mysql
         end
       end
     end
 
-    def check_add_column(args)
-      table, column, type, options = args
-      options ||= {}
+    def check_add_column(*args)
+      options = args.extract_options!
+      table, column, type = args
       default = options[:default]
 
       if !default.nil? && !adapter.add_column_default_safe?
         if options[:null] == false
           options = options.except(:null)
@@ -71,50 +71,50 @@
     # it's okay to allow if the table is empty, but not a fan of data-dependent checks,
     # since the data in production could be different from development
     #
     # note: adding foreign_keys with create_table is fine
     # since the table is always guaranteed to be empty
-    def check_add_foreign_key(args)
-      from_table, to_table, options = args
-      options ||= {}
+    def check_add_foreign_key(*args)
+      options = args.extract_options!
+      from_table, to_table = args
 
       validate = options.fetch(:validate, true)
       if postgresql? && validate
         if StrongMigrations.safe_by_default
-          safe_add_foreign_key(from_table, to_table, options)
+          safe_add_foreign_key(*args, **options)
           throw :safe
         end
 
         raise_error :add_foreign_key,
           add_foreign_key_code: command_str("add_foreign_key", [from_table, to_table, options.merge(validate: false)]),
           validate_foreign_key_code: command_str("validate_foreign_key", [from_table, to_table])
       end
     end
 
-    def check_add_index(args)
-      table, columns, options = args
-      options ||= {}
+    def check_add_index(*args)
+      options = args.extract_options!
+      table, columns = args
 
       if columns.is_a?(Array) && columns.size > 3 && !options[:unique]
         raise_error :add_index_columns, header: "Best practice"
       end
 
       # safe to add non-concurrently to new tables (even after inserting data)
       # since the table won't be in use by the application
       if postgresql? && options[:algorithm] != :concurrently && !new_table?(table)
         if StrongMigrations.safe_by_default
-          safe_add_index(table, columns, options)
+          safe_add_index(*args, **options)
           throw :safe
         end
 
         raise_error :add_index, command: command_str("add_index", [table, columns, options.merge(algorithm: :concurrently)])
       end
     end
 
-    def check_add_reference(method, args)
-      table, reference, options = args
-      options ||= {}
+    def check_add_reference(method, *args)
+      options = args.extract_options!
+      table, reference = args
 
       if postgresql?
         index_value = options.fetch(:index, true)
         concurrently_set = index_value.is_a?(Hash) && index_value[:algorithm] == :concurrently
         bad_index = index_value && !concurrently_set
@@ -125,11 +125,11 @@
           else
             options = options.merge(index: {algorithm: :concurrently})
           end
 
           if StrongMigrations.safe_by_default
-            safe_add_reference(table, reference, options)
+            safe_add_reference(*args, **options)
             throw :safe
           end
 
           if options.delete(:foreign_key)
             headline = "Adding a foreign key blocks writes on both tables."
@@ -146,13 +146,13 @@
             append: append
         end
       end
     end
 
-    def check_change_column(args)
-      table, column, type, options = args
-      options ||= {}
+    def check_change_column(*args)
+      options = args.extract_options!
+      table, column, type = args
 
       safe = false
       existing_column = connection.columns(table).find { |c| c.name.to_s == column.to_s }
       if existing_column
         existing_type = existing_column.sql_type.sub(/\(\d+(,\d+)?\)/, "")
@@ -166,11 +166,11 @@
       end
 
       raise_error :change_column, rewrite_blocks: adapter.rewrite_blocks unless safe
     end
 
-    def check_change_column_null(args)
+    def check_change_column_null(*args)
       table, column, null, default = args
       if !null
         if postgresql?
           safe = false
           safe_with_check_constraint = adapter.server_version >= Gem::Version.new("12")
@@ -240,25 +240,33 @@
 
     def check_change_table
       raise_error :change_table, header: "Possibly dangerous operation"
     end
 
-    def check_create_table(args)
-      table, options = args
-      options ||= {}
+    def check_create_join_table(*args)
+      options = args.extract_options!
 
       raise_error :create_table if options[:force]
 
-      # keep track of new tables of add_index check
+      # TODO keep track of new table of add_index check
+    end
+
+    def check_create_table(*args)
+      options = args.extract_options!
+      table, _ = args
+
+      raise_error :create_table if options[:force]
+
+      # keep track of new table of add_index check
       @new_tables << table.to_s
     end
 
     def check_execute
       raise_error :execute, header: "Possibly dangerous operation"
     end
 
-    def check_remove_column(method, args)
+    def check_remove_column(method, *args)
       columns =
         case method
         when :remove_timestamps
           ["created_at", "updated_at"]
         when :remove_column
@@ -286,23 +294,29 @@
         code: code,
         command: command_str(method, args),
         column_suffix: columns.size > 1 ? "s" : ""
     end
 
-    def check_remove_index(args)
-      table, options = args
-      unless options.is_a?(Hash)
-        options = {column: options}
-      end
-      options ||= {}
+    def check_remove_index(*args)
+      options = args.extract_options!
+      table, _ = args
 
       if postgresql? && options[:algorithm] != :concurrently && !new_table?(table)
+        # avoid suggesting extra (invalid) args
+        args = args[0..1] unless StrongMigrations.safe_by_default
+
+        # Active Record < 6.1 only supports two arguments (including options)
+        if args.size == 2 && ar_version < 6.1
+          # arg takes precedence over option
+          options[:column] = args.pop
+        end
+
         if StrongMigrations.safe_by_default
-          safe_remove_index(table, options)
+          safe_remove_index(*args, **options)
           throw :safe
         end
 
-        raise_error :remove_index, command: command_str("remove_index", [table, options.merge(algorithm: :concurrently)])
+        raise_error :remove_index, command: command_str("remove_index", args + [options.merge(algorithm: :concurrently)])
       end
     end
 
     def check_rename_column
       raise_error :rename_column