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