Migrations w/o down methods irreversible by default

Posted here as it's a somewhat opinionated patch.

http://dev.rubyonrails.org/ticket/9625 why have:

class DoSomethingNastyToTheDatabase > ActiveRecord::Migration    def self.up      execute "some nasty database call"    end

   def self.down      raise ActiveRecord::IrreversibleMigration    end end when you could simply do:

class DoSomethingNastyToTheDatabase > ActiveRecord::Migration    def self.up      execute "some nasty database call"    end end

Attached to the ticket is a patch to implement this, keep backwards compatibility, improve docs and improve test coverage.

-- tim

I support this, makes it behave just as I would expect.

BTW, syntax highlighting in Trac? Never knew about that! http://dev.rubyonrails.org/wiki/WikiProcessors

Posted here as it's a somewhat opinionated patch.

This doesn't match my usage pattern. Usually my migrations that just go up are about fixing data or something else, like populating a counter cache. I still want to be able to go 3 versions back, but if there's an irreversible mixed in there it'll halt.

So then what is the point of the IrreversibleMigration exception? If you are going to force people to create a rollback then that exception should not exist.

IMO if we're going to have an IrreversibleMigration exception, we should allow one to omit the down method. Sorry for not specifying :slight_smile:

It seems safer to assume that a missing down is equivalent to "either there's no way to reverse this, or I didn't think about how to reverse this", which this patch would do.

An empty down would signify "reversing this is a no-op", and it seems like that would still work with this patch...

Agreed. There should also be an exception IDidntThinkAboutTheReverse :slight_smile:

-1

I don't see that there is anything that needs fixing here. This seems like a perfect fit for a plugin.

Yeah I actually have your workflow too. I'm not talking about changing behaviour and removing the self.down from the default template, just improving the use case of creating an IrreversibleMigration.

I guess really there's 3 categories of migrations:

(1) Up + down, reversible (2) Up only, reversible (3) Up only, irreversible

At the moment things are set up so (1) and (2) are easy peasy (from the generator template), but (3) is harder than it should be. Why not just remove self.down?

At the moment (3) requires looking up the (currently incorrect) API instructions and writing a message why you're raising.

As an aside: it seems odd/useless to me that the IrreversibleMigration class is defined and then only referenced in the documentation.... it's not actually handled anywhere. You could raise any old error.

-- tim

pfft I wouldn't even bother with a plugin. A quick suggestion that I thought might ring a note. If it doesn't, then let it die a quick death.

Its a small thing, but it's the small things yakyakyak

-- tim

I like the idea of a migration without a down method being irreversible, and sympathise with some of the feelings of inelegance when raising an exception.

> This doesn't match my usage pattern. Usually my migrations that just > go up are about fixing data or something else, like populating a > counter cache. I still want to be able to go 3 versions back, but if > there's an irreversible mixed in there it'll halt.

Since the migration generator creates blank down methods, most people won't be affected at all. Migrations that do raise exceptions will still halt; migrations that are lacking down methods but which ARE reversible can simply have them added and any stalled migration can continue thereafter.

As an aside: it seems odd/useless to me that the IrreversibleMigration class is defined and then only referenced in the documentation.... it's not actually handled anywhere. You could raise any old error.

For me, this is as good a reason as any to reconsider how migrations handle reversibility. Who else has scratched their head trying to remember what exception we ought to throw to prevent migrations down from the current point? And it doesn't even matter! We could just as easily call exit(-1) (and output something to stderr if we're feeling generous).

For me, this is as good a reason as any to reconsider how migrations handle reversibility. Who else has scratched their head trying to remember what exception we ought to throw to prevent migrations down from the current point? And it doesn't even matter! We could just as easily call exit(-1) (and output something to stderr if we're feeling generous).

I'll confess to having more than one migration with nil.asdf in the down method simply because I don't remember how to correctly mark them as irreversible... However I think I would prefer to encourage users to explain WHY something isn't reversible, if just for the other users.

It would beinteresting if the irreversible migration accepted a reason attribute and printed that reason when the exception is raised in console. Perhaps this would be a way to make self down useful in situations where all you are doing is raising the exception.

Perhaps we could get most of that by just having a specific rescue block for IrreversibleMigration which printed out a nice error message? Anyone care to take a stab at it?

Sure, here's a stab: http://dev.rubyonrails.org/ticket/9634

Manfred