This project is archived and is in readonly mode.

#6339 ✓wontfix
Akira Matsuda

sexier migration that doesn't need the block parameter for create_table

Reported by Akira Matsuda | January 26th, 2011 @ 11:52 PM

This patch enables the following syntax for AR::TableDefinition#create_table.

class CreateUsers < ActiveRecord::Migration
  def change
    create_table :users do
      string :name
      timestamps
    end
  end
end

Traditional syntax is still available. So, this patch does not break any existing migration files.

class CreateUsers < ActiveRecord::Migration
  def change
    create_table :users do |t|
      t.string :name
      t.timestamps
    end
  end
end

I believe the new syntax is more Rails-3-ish, I mean, consistent Rails 3 API, since the routes DSL and ActionMailer DSL also dropped their block parameters in Rails 3.

And, I think 3.1 is the best timing to apply this change because some other changes were already introduced to the migration DSL.

Comments and changes to this ticket

  • Rohit Arondekar

    Rohit Arondekar January 27th, 2011 @ 03:44 AM

    • Importance changed from “” to “Low”

    I like it. It does fit in more with the Rails 3 way of doing things.

    I'd love it if we can get more people to review the patch or give their opinions so that we can assign it to a core member for review. :)

  • Jesse Storimer

    Jesse Storimer January 27th, 2011 @ 04:01 AM

    I'm not core, but I think it's great.

  • Prem Sichanugrist (sikachu)

    Prem Sichanugrist (sikachu) January 27th, 2011 @ 04:28 AM

    I think I like this patch. It really fits the trend of Rails 3 as in the route, where we remove map. from the route file.

    So yeah, +1. Would love to get this applied.

    Aaron, can I have your opinion too?

  • lakshmanan
  • Jan Jones
  • Rohit Arondekar

    Rohit Arondekar January 27th, 2011 @ 12:27 PM

    • State changed from “new” to “open”
    • Assigned user set to “José Valim”

    Ok, assigning to a core member for consideration. :)

  • José Valim

    José Valim January 27th, 2011 @ 12:37 PM

    • State changed from “open” to “wontfix”

    Thanks for the patch, but I have -1.

    Using instance_eval changes the self.scope and usually causes a lot of confusion. For that reason, Rails avoids using instance_eval whenever it can.

    In a few places, like the router, it is convenient because we usually nest several blocks, one inside the other. But this is not the case for migrations. So I will stick with simplicity.

  • Akira Matsuda

    Akira Matsuda January 27th, 2011 @ 02:55 PM

    @José

    Thanks for reviewing!
    I think I understand your point. Yes, eval family should not be abused. It may cause unexpected weird problem when combined with other metaprogramming technique.
    However, I think it won't cause any problem in this paricular case, because we're talking about migrations.

    Methods we use inside the create_table block are besically restricted to the following 15 methods.

    # You see this migration command surely runs on my console.
    ActiveRecord::Schema.define do
      create_table :people do
        column :string, :col1
        timestamps
        references :model1
        belongs_to :model2
        string :col2
        text :col3
        integer :col4
        float :col4
        decimal :col5
        datetime :col6
        timestamp :col7
        time :col8
        date :col9
        binary :col10
        boolean :col11
      end
    end
    
    -- create_table(:people)
       -> 0.0060s
    

    That is why instance_eval is already used in AR::Schema.define without causing any problem ever. https://github.com/rails/rails/blob/master/activerecord/lib/active_...
    Who does metaprogramming inside create_table block in migration files?

    So, what do you actually worry? Am I missing any possible usecase?

  • Akira Matsuda

    Akira Matsuda January 27th, 2011 @ 03:04 PM

    # sorry I made a tiny mistake
    s/column :string, :col1/column :col1, :string/
    
  • José Valim

    José Valim January 27th, 2011 @ 03:41 PM

    • Assigned user changed from “José Valim” to “Aaron Patterson”

    Ok, I am on the fence. :) Aaron Patterson is responsible for AR so I will defer the final decision to him.

  • Matt Kern

    Matt Kern May 20th, 2011 @ 04:44 AM

    +1

    I'm for the simplification for sure. And, though I completely agree that instance_eval can be problematic, in this case it seems totally worthwhile.

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>

Attachments

Pages