Rails Ruby on Rails

Comments and changes to this ticket

• 

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

• You flagged this item as spam.
  

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

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

• 

  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?

• You flagged this item as spam.
  

  lakshmanan January 27th, 2011 @ 08:42 AM

  +1

  looks cool.

• You flagged this item as spam.
  

  Jan Jones January 27th, 2011 @ 08:47 AM

  +1

• 

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

• You flagged this item as spam.
  

  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?

• You flagged this item as spam.
  

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

  # sorry I made a tiny mistake
  s/column :string, :col1/column :col1, :string/
  

• 

  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.

• You flagged this item as spam.
  

  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.