Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sexier migrations #1163

Merged
merged 2 commits into from Nov 4, 2011
Merged

Sexier migrations #1163

merged 2 commits into from Nov 4, 2011

Conversation

amatsuda
Copy link
Member

Let me propose a new Rails-3-ish cooler syntax for the database migrations.
This pull request enables you to remove the block parameter and t. t. t. t. t. t. t. ... from your migration code.

This is actually the same thing with what I posted here in January, so please refer to this ticket for details.
https://rails.lighthouseapp.com/projects/8994/tickets/6339

The reasons why I made another pull request today are:

  • The attached patch on the ticket does not apply anymore to the current master
  • The proposal got many +1s from people
  • The ticket is in "wontfix" state (by mistake?), and maybe because of that, was not copied to GH issues
  • I think 3.1 is the best timing for including another change on the migration syntax
  • I guess @tenderlove have not looked at it yet

So, @tenderlove, can you please take a look at the former ticket and this pull request, then give me your thoughts?
Thanks in advance!

Now you can omit |t| block parameter and all the t. from your migration code, that means, the syntax looks more Rails-3-ish, like the routes DSL and ActionMailer DSL.
Also, this change won't break any of your existing migration files, since the traditional syntax is still available.
@fxn
Copy link
Member

fxn commented May 20, 2011

If this pull request is applied I think this idiom should become the recommended one, and thus all example code in documentation would need to be updated to reflect that. Only the API docs of create_table would mention the block may receive a argument.

@amatsuda
Copy link
Member Author

Yes, agree. And of course I'm willing to work on that when I'm sure the syntax will be accepted.
I mean, I just wanted to know first whether you like it or not, before updating the whole documentation.

@asanghi
Copy link
Contributor

asanghi commented May 20, 2011

+1, looks sweet. Lets do this if we can.
We've got instance evals in routes already as a pre-cursor.
Migrations aren't run very frequently so there would be negligible net performance impact.
We're already updating the migration syntax in Rails 3.1 so why not go a step further.
It's backward compatible as far as i can see.

@joeljunstrom
Copy link

+1 for simplified api, definitely have a more "Rails 3 feel"

@Fonsan
Copy link
Contributor

Fonsan commented May 20, 2011

asanghi what he says +1

@jonleighton
Copy link
Member

I also give this a 👍, but it is too late for it to go into 3.1 so we should return to this once 3.1 is out the door.

@josevalim
Copy link
Contributor

I am in general -1 for instance eval. instance eval has its set of gotchas that does not make it worth imo.

@tenderlove
Copy link
Member

I'm OK with this change, but I want to see what methods we would expose by doing instance_eval before I full agree with this change. Doing the instance eval will allow private methods to be exposed on the table_definition object, and that kind of worries me.

In any case, I agree with @jonleighton that this should be considered for 3.2. I wish somebody would branch for 3.1 so we can start merging in 3.2 features. :-P

@myronmarston
Copy link
Contributor

Given the instance_eval gotchas that @josevalim mentioned, and the fact that there are tons of migrations out in the wild with the old syntax, I think both ways should be supported. That way, old migrations do not break, and people who prefer the non-instance_eval syntax and still choose to avoid it. Here's a simple way to accomplish that:

if blk.arity == 1
  blk.call(td)
else
  td.instance_eval(&blk)
end

@plentz
Copy link
Contributor

plentz commented May 20, 2011

+1

@asanghi
Copy link
Contributor

asanghi commented May 20, 2011

We ❤️ this already. The only stuff that really would later are a few blog posts as far as dissemination of information is concerned. Why wait til 3.2 to introduce yet more changes in migrations. If we're going to have this might as well make the changes now, still not too late. 👍 on @myronmarston's changes.

@spastorino
Copy link
Contributor

3.1 is feature complete. I have the same concern that @tenderlove has, it could be nice to see what are we going to expose to the table_definition objects.

@radar
Copy link
Contributor

radar commented May 22, 2011

-1, I'd prefer we weren't adding extra code for the sake of making this two characters shorter per line

@dasch
Copy link
Contributor

dasch commented May 25, 2011

+1, this seems in line with the direction Rails has taken, e.g. in the routing DSL.

@TheEmpty
Copy link
Contributor

@radar please compare the time that it takes for a machine to interpret these lines, what three or four lines, and the time for a human to write those two characters. You'll find that this is +1

+1 :)

jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011
Signed-off-by: Frederick Cheung <frederick.cheung@gmail.com>
@elomarns
Copy link

+1.

Cleaner interfaces is always best.

@ghost ghost assigned tenderlove Oct 9, 2011
@jeremy
Copy link
Member

jeremy commented Oct 9, 2011

This is a good candidate for 3.2, let's merge to master and update documentation.

@jeremy
Copy link
Member

jeremy commented Oct 24, 2011

This is green for 3.2, we just need docs/guides updates.

@soulnafein
Copy link

+1
There isn't really much extra code, and the migrations look nicer.

tenderlove added a commit that referenced this pull request Nov 4, 2011
@tenderlove tenderlove merged commit 0e407a9 into rails:master Nov 4, 2011
@vijaydev
Copy link
Member

vijaydev commented Nov 4, 2011

FTR this doesn't have documentation. @tenderlove what about this suggestion?#1163 (comment)

@tenderlove
Copy link
Member

@vijaydev the suggestion seems good. Can you put together a pull request with tests? Thanks!

tenderlove added a commit that referenced this pull request Nov 17, 2011
This reverts commit 0e407a9, reversing
changes made to 533a9f8.

Conflicts:

	activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
	activerecord/test/cases/migration_test.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet