This project is archived and is in readonly mode.

#71 ✓resolved
Jeff Dean

[patch] change_table block

Reported by Jeff Dean | May 1st, 2008 @ 03:06 AM

I created a fork that adds change_table to migrations like so:

change_table :videos do |t|
  t.add_timestamps
  t.add_belongs_to :goat
  t.add_string :name, :email, :limit => 20
  t.remove_column :name, :email
  t.string :some_string
  t.rename :some_new_name
  t.drop
end

This brings Sexy Migrations to the change table helpers as well, in the same style as create_table.

It contains wrappers for all of the migration functions, tests and docs. Some have metioned that "rename" and "drop" are confusing, but I find the alternatives confusing. I'd be open to removing "rename" and "drop" and updating the tests and docs if that's necessary.

This also makes remove_column take an array, so that the sexy migration "t.string :name, :email" has a counterpart "remove_column(s) :name, :email"

$ git remote add zilkey git://github.com/zilkey/rails.git
$ git checkout -b zilkey/master
$ git pull zilkey

(or however you normally pull requests)

I'd be happy to provide and upload a patch as well, but now that github exists, I can't see any reason to (pulling seems much faster and easier). But if not having a patch is a blocker, I'll upload one.

Feedback so far has been tepid, but I think this is a strong and useful addition, one that I use regularly now in production. It's a big feature to add, but it's very well tested and documented.

Comments and changes to this ticket

  • DHH

    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

    DHH May 1st, 2008 @ 05:27 AM

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

  • Jeff Dean

    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.

  • Jeff Dean

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

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

    Here's the verbose patch.

  • DHH

    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.

  • Jeff Dean

    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.

  • Jeff Dean
  • Repository

    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...

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Referenced by

Pages