Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #9166 (closed enhancement: fixed)

Opened 1 year ago

Last modified 10 months ago

[PATCH] Migration generator should accept attribute pairs

Reported by: lifofifo Assigned to: core
Priority: normal Milestone: 1.x
Component: Railties Version: edge
Severity: normal Keywords:
Cc:

Description

Changeset [7216] introduced a new convention for generating migrations. It was good, but however, not very useful.

This patch extends the idea further to add optional list of attribute pairs as arguments.

So now when you do : 

script/generate migration AddMoreToPost title:string body:text published:boolean

Generated migration will look like : 

class AddMoreToPost < ActiveRecord::Migration
  def self.up
    add_column :posts, :title, :string  
    add_column :posts, :body, :text  
    add_column :posts, :published, :boolean  
  end

  def self.down
    remove_column :posts, :published  
    remove_column :posts, :body  
    remove_column :posts, :title  
  end
end

The patch also removes the functionality of Changeset [7216] as I believe this approach is a better one.

Thanks.

Attachments

pretty_migration_generator.patch (4.3 kB) - added by lifofifo on 08/02/07 02:20:23.
A new convention for migration generator
pretty_migration_generator.2.patch (5.8 kB) - added by kampers on 08/03/07 07:47:45.
allows attribute lists for CreateTable and DropTable as well
pretty_migration_generator.3.patch (4.9 kB) - added by lifofifo on 08/03/07 11:24:02.
Support CreateTable

Change History

08/02/07 02:20:23 changed by lifofifo

  • attachment pretty_migration_generator.patch added.

A new convention for migration generator

08/02/07 02:43:40 changed by danger

Is there any reason why this can't coincide with Zenspider's auto migration patch? It's a very common convention to create migrations named AddNameToPost. Since we also already have the convention of attribute pairs specified on the command line when creating scaffolds it seems like the two should both be allowed to stay.

I'd like to see a patch that allows me to do this: 

./script/generate migration -c AddNameToPost
./script/generate migration -c AddAuthorIdAndContentToPost author_id:integer content:text

If I could do that and have it all generated for me I'd be a happy camper.

08/02/07 03:09:27 changed by lifofifo

As you agree, we already have convention of attribute pairs for model generator and scaffolding. Then why introduce a new convention for migrations when there are no significant gains ?

I'm sure doing something like "script/generate migration AddNameToPost" and then open migration file and fill in proper details and then run "rake db:migrate" will be a lot slower than "script/generate migration AddNameToPost name:string && rake db:migrate".

And then we have patch requests like 9153.

Zenspider's auto migration thing just doesn't seem to fit in well with rest of the stuff and it really doesn't do something very useful to be an exceptional case.

With this patch, I see no need of the previous behavior.

08/02/07 03:43:14 changed by danger

Hrmm, I think you make a pretty convincing argument. I suppose I could get used to specifying everything on the command line.

(follow-up: ↓ 5 ) 08/03/07 07:46:24 changed by kampers

  • summary changed from [PATCH] Pretty migration generator to [PATCH] Migration generator should accept attribute pairs.

Yeah, I'm with lifo on this one; not having this (and having [7216] instead) violates the principle of least surprise. I have anecdotal evidence to boot--I tried to pass attribute pairs to the migration generator the other day, not realizing that it didn't accept them. [7216] is cute, but I think we should be consistent with the rest of the generators that infer attributes from the command line.

With that said, I think that if we go this far, we should also add CreateTable and DropTable. So I did, here's a new patch.

Some other notes:

  • added the banner method to suggest [field:type, field:type] like the Model generator does
  • rewrote/reformatted USAGE a bit
  • with CreateTable and DropTable added, the template is getting a little messy. I did some creative indenting (like <% code %>) to try and make it more readable, but it seems like we might need to refactor some of the logic out of the template and back into migration_generator.rb as it was in [7216].

08/03/07 07:47:45 changed by kampers

  • attachment pretty_migration_generator.2.patch added.

allows attribute lists for CreateTable and DropTable as well

(in reply to: ↑ 4 ) 08/03/07 11:23:33 changed by lifofifo

Replying to kampers:

With that said, I think that if we go this far, we should also add CreateTable and DropTable. So I did, here's a new patch.

Nice point. Though I don't think DropTable will be very useful as it's very likely that you'll be raising ActiveRecord::IrreversibleMigration exception whenever you drop the table, and also not a good idea to supply attributes from cli to DropTable.

I've attached a new patch that supports CreateTable form by resuing template in model/templates.

Thanks.

08/03/07 11:24:02 changed by lifofifo

  • attachment pretty_migration_generator.3.patch added.

Support CreateTable

08/05/07 00:11:39 changed by mislav

+1 for the table stuff, feels great

My complaint is about AddTitleBodyToPost. I think it should be AddTitleAndBodyToPost for consistency with Rails stuff like dynamic AR finders (find_by_title_and_body). Also, if column names in the migration title are separated by "and" then the generator can know exactly what columns are we adding and implicitly assume they are string columns (unless specified explicitly like "body:text"). If they are not separated by "and" then the generator can't tell if we want to add "title" and "body" or "title_body" column.

08/24/07 10:22:04 changed by court3nay

-1, let's see the plugin please. there are too many dissenting calls for syntax.

08/24/07 13:48:12 changed by lifofifo

court3nay: I don't really see any sense in having a plugin for migration generator. This is a tiny, but quite useful feature, which leverages existing functionality. And ohhh, did I tell you it's already there ;-) ? Too late for a plugin.

08/24/07 17:03:39 changed by danger

+1 from me. I'm convinced now that attribute pairs (even if I don't use them) should be acknowledged on migration generation. It's consistent with other generators and is a more complete solution than [7216].

09/09/07 05:40:38 changed by nzkoz

  • status changed from new to closed.
  • resolution set to fixed.

(In [7422]) Use attribute pairs instead of the migration name to create add and remove column migrations. Closes #9166 [lifofifo]