Rails Ruby on Rails

Comments and changes to this ticket

• 

  DHH May 1st, 2008 @ 04:40 AM

  Thoughts: Make the add_ implicit, just like in create_table, so you do t.string :column to add a column. Then you can drop column from remote for just doing t.remove :column. I don't think drop and rename fit well. Otherwise, I like it!

• 

  DHH May 1st, 2008 @ 05:27 AM

  Also, pulling is actually a big hassle. Please do upload a patch.

• You flagged this item as spam.
  

  Jeff Dean May 1st, 2008 @ 10:13 AM

  I agree that removing add_ makes it much clearer. However, removing those makes the naming a bit less clear to me. I've made those changes and provided 2 patches here so that you can choose the style you like the most:

  • Regular Version which omits the word "column" and looks more like create_table with:

  ** t.change

  ** t.change_default

  ** t.rename

  • Verbose Version which retains the word "column" and looks more like regular change helpers with:

  ** t.change_column

  ** t.change_column_default

  ** t.rename_column

  Each patch has appropriate passing tests and documentation and are identical except for the 3 methods mentioned above.

  The regular patch produces all of the following methods:

  change_table :table do |t|
   t.column
   t.index
   t.timestamps
   t.change
   t.change_default
   t.rename
   t.references
   t.belongs_to
   t.string 
   t.text 
   t.integer 
   t.float 
   t.decimal 
   t.datetime 
   t.timestamp 
   t.time 
   t.date 
   t.binary 
   t.boolean
   t.remove
   t.remove_references
   t.remove_belongs_to
   t.remove_index
   t.remove_timestamps
  end
  

  The verbose patch produces all of the following methods:

  change_table :table do |t|
    t.column
    t.index
    t.timestamps
    t.change_column
    t.change_column_default
    t.rename_column
    t.references
    t.belongs_to
    t.string 
    t.text 
    t.integer 
    t.float 
    t.decimal 
    t.datetime 
    t.timestamp 
    t.time 
    t.date 
    t.binary 
    t.boolean
    t.remove
    t.remove_references
    t.remove_belongs_to
    t.remove_index
    t.remove_timestamps
  end
  

  I've removed the helpers for "drop_table" and "rename_table". In both patches, "remove_column" now takes an array of field names, with updated tests.

• You flagged this item as spam.
  

  Jeff Dean May 1st, 2008 @ 10:14 AM

  (the list formatting got eaten on that last post...)

  Here's the verbose patch.

• 

  DHH May 1st, 2008 @ 11:45 PM

  Getting the following failures on rake test_sqlite3 in AR after applying:

  1) Failure:

  test_integer_creates_integer_column(ChangeTableMigrationsTest)

  [(eval):1:in `add_column'

  (eval):16:in `integer'

  (eval):5:in `each'

  (eval):5:in `integer'

  ./test/cases/migration_test.rb:1253:in `test_integer_creates_integer_column'

  ./test/cases/migration_test.rb:1345:in `with_change_table'

  ./test/cases/../../lib/active_record/connection_adapters/abstract/schema_statements.rb:165:in `change_table'

  ./test/cases/migration_test.rb:1344:in `with_change_table'

  ./test/cases/migration_test.rb:1250:in `test_integer_creates_integer_column'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run']:

  #.add_column(:delete_me, :foo, 'integer', {}) - expected calls: 0, actual calls: 1

  Similar expectations:

  add_column(:delete_me, :foo, 'int(11)', {})

  add_column(:delete_me, :bar, 'int(11)', {})

  2) Failure:

  test_add_remove_single_field_using_an_array_of_arguments(MigrationTest)

  [./test/cases/migration_test.rb:471:in `test_add_remove_single_field_using_an_array_of_arguments'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run']:

  is not true.

  3) Failure:

  test_add_remove_single_field_using_multiple_string_arguments(MigrationTest)

  [./test/cases/migration_test.rb:423:in `test_add_remove_single_field_using_multiple_string_arguments'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run']:

  is not true.

  4) Failure:

  test_add_remove_single_field_using_multiple_symbol_arguments(MigrationTest)

  [./test/cases/migration_test.rb:441:in `test_add_remove_single_field_using_multiple_symbol_arguments'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run']:

  is not true.

  5) Failure:

  test_remove_columns_works_like_remove_column(MigrationTest)

  [./test/cases/migration_test.rb:477:in `test_remove_columns_works_like_remove_column'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `__send__'

  ./test/cases/../../lib/../../activesupport/lib/active_support/testing/setup_and_teardown.rb:59:in `run']:

  is not true.

• You flagged this item as spam.
  

  Jeff Dean May 2nd, 2008 @ 04:58 AM

  • Title changed from “Add change_table to core?” to “[patch] change_table block”

  Sorry to have taken your time with that - I've updated the patches to include tests that run in:

  • MySQL
  • PostgreSQL
  • SQLite3

  I don't have any of the other databases installed, so I can't vouch for those, but SQLite should work as well.

  • change_table_with_passing_tests.patch 20.6 KB
• 

  Jeff Dean May 2nd, 2008 @ 04:58 AM

  • no changes were found...

  • change_table_verbose_with_passing_tests.patch 20.5 KB
• 

  Repository May 3rd, 2008 @ 05:30 PM

  • State changed from “new” to “resolved”

  (from [96980bd561d79824b6cb6efbcbecdcbf8785d452]) Added change_table for migrations (Jeff Dean) [#71 state:resolved]

  http://github.com/rails/rails/co...