search

SchemaPlus / schema_validations

Automatically creates validations basing on the database schema.
Other
174 stars 34 forks source link

Update for Rails 5 #44

Closed plicjo closed 8 years ago

plicjo commented 8 years ago

This PR has all of the schema_validations tests passing for Rails 5 and Rails 4.2.

Cheers!

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.005%) to 99.259% when pulling edb5a63190c1bf5b060eac81113f4dce642254ab on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 3925aa9c7a49718fc623c6ab17a894d8dec50551 on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 283cc7cb3a05a694affb7bf0cb31236468710dde on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 283cc7cb3a05a694affb7bf0cb31236468710dde on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling b615555e5c3f23d8ca248f6cfa2b96cd3d91e39c on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling b615555e5c3f23d8ca248f6cfa2b96cd3d91e39c on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling b615555e5c3f23d8ca248f6cfa2b96cd3d91e39c on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 853763217186a2d75ba7e899e684a7c12186774e on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 853763217186a2d75ba7e899e684a7c12186774e on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.7%) to 100.0% when pulling 853763217186a2d75ba7e899e684a7c12186774e on plicjo:postgres_upgrade_to_rails_5 into b3e8be26a076e7e138a8c73df2be8266fc13ad63 on SchemaPlus:master.

boazy commented 8 years ago

@plicjo Great job with the pull request. I've also started working on adapting schema_validations, but you obviously beat me to it. :)

I'm now reviewing it, but as far as tests and coverage go it looks ready. Besides that, the main issue I see right now, is that the detected types don't necessarily match the ActiveModel types mapped to the database. Changes in Rails 5 mean we cannot call #number? on types, and we don't get the casted type directly so we could call #number? or #range? on.

In the second case, I haven't seen any example of rails using anything besides an integer range, so that might be just fine. With numeric types things are more complicated, since Postgres has the Money type, for instance, and it looks like it won't be detected as numeric. Then again, the old code might have been wrong too, since it detected Money as numeric, but not as decimal.

I have a workaround ready from my own attempts that allows us to check for #numeric? in a compatible way, but I'm not sure if it actually solves the issue. Perhaps we should just add explicit mappings for types like :money instead.

@ronen Thoughts?

plicjo commented 8 years ago

@boazy Thank you! If you give me a list of the missing, unsupported types, I can take care of it.

I took a look at the Rails source code, and I also came to the conclusion that the #range check appears to only be concerned with integers.

boazy commented 8 years ago

With the current version of Rails it seems that the only missing type is :money from PostgreSQL which should be mapped to Decimal. I'm more concerned about future types but this particular part of Rails seems prone to breaking changes no matter which implementation we choose.

ronen commented 8 years ago

Thanks @plicjo for taking this on! And thanks @boazy for looking into it.

I'd say if the tests pass, we're OK to release. Support for other types and whatnot can always be added in future releases. Do you agree?

boazy commented 8 years ago

I agree, if we convert money to decimal of course. That alone is a regression, although the currwnt behavior ia not fully correct either. This is jus one line.

ronen commented 8 years ago

Yeah, certainly may as well fix that. Since the current behavior for money is broken, instead of considering it a regression, should we consider it to be a bug fix? And so this would be a patch release rather than major?

boazy commented 8 years ago

Let's merge and then apply the bugfix then.

plicjo commented 8 years ago

I opened an issue for adding support for the Postgres :money column type. https://github.com/SchemaPlus/schema_validations/issues/45

boazy commented 8 years ago

Thanks @plicjo. I've merged your pull request and I'll try to fix that issue today.

boazy commented 8 years ago

@ronen Can you please release a new version?

ronen commented 8 years ago

Sorry, I got confused and thought I was waiting for another fix from you. Just realized now it's all set (when #47 came in).

Released 2.2.0

boazy commented 8 years ago

Thanks @ronen! I'll test the new schema_validations today in a live project ;) Doy we have any major modules left without Rails 5 support now besides schema_plus_views?