Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Differentiate between remove_column and remove_columns. Make remove_c…

…olumn reversible.

[#8267]
  • Loading branch information...
commit e28ddea098d0422bae08324fb1d72f2b100152d0 1 parent af871a0
@marcandre marcandre authored
View
2  activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb
@@ -423,7 +423,7 @@ def change_default(column_name, default)
# t.remove(:qualification)
# t.remove(:qualification, :experience)
def remove(*column_names)
- @base.remove_column(@table_name, *column_names)
+ @base.remove_columns(@table_name, *column_names)
end
# Removes the given index from the table.
View
25 activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -321,14 +321,26 @@ def add_column(table_name, column_name, type, options = {})
execute(add_column_sql)
end
- # Removes the column(s) from the table definition.
+ # Removes the given columns from the table definition.
#
- # remove_column(:suppliers, :qualification)
# remove_columns(:suppliers, :qualification, :experience)
- def remove_column(table_name, *column_names)
- columns_for_remove(table_name, *column_names).each {|column_name| execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{column_name}" }
+ def remove_columns(table_name, *column_names)
+ raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.empty?
+ column_names.each do |column_name|
+ remove_column(table_name, column_name)
+ end
+ end
+
+ # Removes the column from the table definition.
+ #
+ # remove_column(:suppliers, :qualification)
+ #
+ # The +type+ and +options+ parameters will be ignored if present. It can be helpful
+ # to provide these in a migration's +change+ method so it can be reverted.
+ # In that case, +type+ and +options+ will be used by add_column.
+ def remove_column(table_name, column_name, type = nil, options = {})
+ execute "ALTER TABLE #{quote_table_name(table_name)} DROP #{quote_column_name{column_name}}"
end
- alias :remove_columns :remove_column
# Changes the column's definition according to the new options.
# See TableDefinition#column for details of the options you can use.
@@ -677,7 +689,8 @@ def index_name_for_remove(table_name, options = {})
end
def columns_for_remove(table_name, *column_names)
- raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.blank?
+ ActiveSupport::Deprecation.warn("columns_for_remove is deprecated and will be removed in the future")
+ raise ArgumentError.new("You must specify at least one column name. Example: remove_columns(:people, :first_name)") if column_names.blank?
column_names.map {|column_name| quote_column_name(column_name) }
end
View
10 activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -444,15 +444,11 @@ def add_column(table_name, column_name, type, options = {}) #:nodoc:
end
end
- def remove_column(table_name, *column_names) #:nodoc:
- raise ArgumentError.new("You must specify at least one column name. Example: remove_column(:people, :first_name)") if column_names.empty?
- column_names.each do |column_name|
- alter_table(table_name) do |definition|
- definition.columns.delete(definition[column_name])
- end
+ def remove_column(table_name, column_name, type = nil, options = {}) #:nodoc:
+ alter_table(table_name) do |definition|
+ definition.columns.delete(definition[column_name])
end
end
- alias :remove_columns :remove_column
def change_column_default(table_name, column_name, default) #:nodoc:
alter_table(table_name) do |definition|
View
9 activerecord/lib/active_record/migration/command_recorder.rb
@@ -73,7 +73,7 @@ def respond_to?(*args) # :nodoc:
[:create_table, :create_join_table, :change_table, :rename_table, :add_column, :remove_column,
:rename_index, :rename_column, :add_index, :remove_index, :add_timestamps, :remove_timestamps,
:change_column, :change_column_default, :add_reference, :remove_reference, :transaction,
- :drop_join_table, :drop_table
+ :drop_join_table, :drop_table, :remove_columns,
].each do |method|
class_eval <<-EOV, __FILE__, __LINE__ + 1
def #{method}(*args, &block) # def create_table(*args, &block)
@@ -114,7 +114,12 @@ def invert_rename_table(args)
end
def invert_add_column(args)
- [:remove_column, args.first(2)]
+ [:remove_column, args]
+ end
+
+ def invert_remove_column(args)
+ raise ActiveRecord::IrreversibleMigration, "remove_column is only reversible if given a type." if args.size <= 2
+ [:add_column, args]
end
def invert_rename_index(args)
View
4 activerecord/test/cases/migration/change_table_test.rb
@@ -173,14 +173,14 @@ def test_change_default_changes_column
def test_remove_drops_single_column
with_change_table do |t|
- @connection.expect :remove_column, nil, [:delete_me, :bar]
+ @connection.expect :remove_columns, nil, [:delete_me, :bar]
t.remove :bar
end
end
def test_remove_drops_multiple_columns
with_change_table do |t|
- @connection.expect :remove_column, nil, [:delete_me, :bar, :baz]
+ @connection.expect :remove_columns, nil, [:delete_me, :bar, :baz]
t.remove :bar, :baz
end
end
View
13 activerecord/test/cases/migration/command_recorder_test.rb
@@ -127,7 +127,18 @@ def test_invert_rename_table
def test_invert_add_column
remove = @recorder.inverse_of :add_column, [:table, :column, :type, {}]
- assert_equal [:remove_column, [:table, :column]], remove
+ assert_equal [:remove_column, [:table, :column, :type, {}]], remove
+ end
+
+ def test_invert_remove_column
+ add = @recorder.inverse_of :remove_column, [:table, :column, :type, {}]
+ assert_equal [:add_column, [:table, :column, :type, {}]], add
+ end
+
+ def test_invert_remove_column_without_type
+ assert_raises(ActiveRecord::IrreversibleMigration) do
+ @recorder.inverse_of :remove_column, [:table, :column]
+ end
end
def test_invert_rename_column
Please sign in to comment.
Something went wrong with that request. Please try again.