Incompatible with schema_plus

[dmeranda]

It appears as if this gem has problems when also used with schema_plus and when both have options for the same column. I'm not sure which gem has a bug; so I also posted an issue over there.

See SchemaPlus/schema_plus#170

Also see that issue for additional details and examples.

[pinnymz]

Thanks for reporting, I'll look into this.

[lowjoel]

With newer versions of Schema Plus, specifically SchemaPlus/schema_monkey#6, db:load/db:reset and friends will cause an infinite recursion. I hypothesise it's due to alias_method_chain being called to early, @ronen and I are looking into this; if that is indeed the problem, would switching over to module includes be okay with you @pinnymz?

If that doesn't work (hence the use of alias_method_chain), would requiring Ruby 2.0 for Module.prepend be an acceptable solution?

[pinnymz]

I'm all for moving to Ruby 2.0 to keep the code lean, and that would include removing alias_method_chain in favor of prepend or the like. That would require a small rewrite, and that is indeed the plan. However, I don't have a timeline on this yet (although now more motivated ;) ).

For the time being, I don't see how using Module includes would solve the problem. Can you possibly provide a sample of such code that demonstrates this?

[lowjoel]

I'm not sure of the exact priority rules for prepend vs include (but I vaguely remember that it's prepended modules [last prepended first], then the class, then included modules [last included first]).

So if a class has a method that is implemented only in a module, including a new module does replace the existing implementation, with the old implementation accessible via super. So it's like a prepend.

ActiveRecord does this, so it might not require a prepend.

[ronen]

@pinnymz, @lowjoel: confirming it's a small rewrite to change from alias_method_chain to override & super, I went through that in schema_plus and it wasn't a big deal.

Personally I'd recommend using prepend rather than trusting subtleties of include order and method definition location. Though that would mean dropping support for ruby 1.9.3 -- but since ruby 1.9.3 is itself no longer supported that doesn't seem too onerous. I took the advice of several people and did that for schema_plus and nobody has complained.

But...

@pinnymz Another thing to consider if you were interested: Re-implement migration_comments on top of schema_plus's infrastructure: schema_plus_core and schema_monkey. I think it would fit right in. Those take care of all the monkey patching to provide an extension API for rails, letting you drop in enhancements like this as middleware without needing to do all that monkey patching yourself.

It'd of course be a bigger change to the code base, which would doubtless give you pause. But once changed there'd actually be much less code, and you'd get upwards-compatibility with future releases of ActiveRecord "for free" when schema_plus_core updates accordingly.

If that's something you're interested in, I'm happy to answer any questions you may have.

[lowjoel]

I think I can push a PR to move this to prepend and super, breaking 1.9 compatibility. @pinnymz would you be okay with it?

If you'd like to use the schema_monkey and schema_plus_core gems, with stuff written using modules+prepend should make it easier to migrate to anyway.

[pinnymz]

@ronen I am giving serious consideration to implementing on top of the schema_plus infrastructure, and I'd like to see what you think this would entail. I think we should take this conversation to another forum.

cc: @lowjoel

[lowjoel]

OK, let me know how it goes.

[ronen]

@pinnymz we never did have that conversation... still interested?

[pinnymz]

@ronen this now seems to be moot, as Rails 5 will be supporting comments. Thanks for the offer, though.

[pinnymz]

@dmeranda #30 uses prepend and should play nicely with other extensions, can you try this out and let me know?

[ronen]

Note that as per SchemaPlus/schema_monkey#13 these gems are compatible but they need to be required in the proper order.