search

SchemaPlus / schema_plus_pg_indexes

Adds support in ActiveRecord for PostgreSQL index expressions and operator classes, as well as a shorthand for case-insensitive indexes
MIT License
30 stars 13 forks source link

add rails 5.2 support #20

Closed urkle closed 5 years ago

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 32ea8c21adb0fcae5b6d36c2cc217abbce65dbd0 on rails-5.2 into cba26617795cdcaf7657128069e4a6cb849492fc on master.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-1.9%) to 98.058% when pulling a7e2d23165a6fa0479b0cabd48525411f6255a77 on rails-5.2 into cba26617795cdcaf7657128069e4a6cb849492fc on master.

lowjoel commented 5 years ago

I'll try to review these, let me know when it's ready for review.

urkle commented 5 years ago

@lowjoel this one passes locally and will pass once a new schema_plus_indexes gem is released.

Now this one does have that "nefarious" Gem::Version check in the main ruby entrypoint file. Mainly because this whole gem is going to become deprecated for 5.2 and not supported after that.

lowjoel commented 5 years ago

No, this makes sense. If we're going to remove a component gem, we should just warn noisily.

I can't remember offhand if everyone's migrations will be run, and I'm just afraid that after upgrading migrations will stop working after Rails 5.2, since I don't imagine migrations to be a particularly well-tested code path.

lowjoel commented 5 years ago

If you can think of a better solution than just telling people to update migrations, I think we should do it. But such a solution eludes me at the moment...

urkle commented 5 years ago

@lowjoel and the way this is coded right now those migrations will still work, and it will yell noisily. :)

I'm actually going to take a peek at paperclip as that one YELLS noisily on install somehow when things change.. And we could have a warning on startup everytime the gem is loaded too.

lowjoel commented 5 years ago

Yeah, I saw all the deprecations. I can't remember the behaviour of AR::Migration though, AFAIK if the migration was already applied before the migration won't be run again, so I'm afraid older migrations may be forgotten (and your warnings won't be tripped)

I'm pretty sure that new migrations are fine.