New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Ohm monkey patch and include Padrino::Ohm::Validator #1196

Merged
merged 8 commits into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
@lastcanal
Contributor

lastcanal commented Mar 28, 2013

This removes the monkey patch to Ohm and includes a module in ./lib/padrino_ohm_validations.rb when a project is generated.

Padrino::Ohm::Validations overrides the error names in Scrivener::Validations to match Active Model's errors. This module is included by default for generated ohm models.

see this 3 year old ohm issue for more info
#1148

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 28, 2013

Contributor

Thanks @lastcanal! Thoughts @skade @ujifgc?

Contributor

dariocravero commented Mar 28, 2013

Thanks @lastcanal! Thoughts @skade @ujifgc?

@lastcanal

This comment has been minimized.

Show comment
Hide comment
@lastcanal

lastcanal Mar 30, 2013

Contributor

Ohm has accepted my pull request and has released 1.3.0. This exposes Model.attributes and allows the admin generator to get a list of attributes from Ohm models. This covers off the second half of the monkey patch.

I have updated the Ohm dependancy to be ~> 1.3.0
I have also included Account.first which allows the admin app 'login bypass' to work with ohm.

Contributor

lastcanal commented Mar 30, 2013

Ohm has accepted my pull request and has released 1.3.0. This exposes Model.attributes and allows the admin generator to get a list of attributes from Ohm models. This covers off the second half of the monkey patch.

I have updated the Ohm dependancy to be ~> 1.3.0
I have also included Account.first which allows the admin app 'login bypass' to work with ohm.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 30, 2013

Contributor

@lastcanal thanks for the update!.. I was glad to see that popping up on Ohm 1.3.0 :). Thanks!..

As for include Padrino::Ohm::Validations I was thinking that perhaps the best would be to drop all of that and modify the admin to comply to it when Ohm is used?

I reckon that would make it simpler and more predictable.

Thoughts?

Contributor

dariocravero commented Mar 30, 2013

@lastcanal thanks for the update!.. I was glad to see that popping up on Ohm 1.3.0 :). Thanks!..

As for include Padrino::Ohm::Validations I was thinking that perhaps the best would be to drop all of that and modify the admin to comply to it when Ohm is used?

I reckon that would make it simpler and more predictable.

Thoughts?

@lastcanal

This comment has been minimized.

Show comment
Hide comment
@lastcanal

lastcanal Mar 31, 2013

Contributor

I'm in favor or dropping Padrino::Ohm::Validations. We could include a rake ohm:translations task for building i18n translation documents with Ohm's error keys.

It would certainly be possible to map all of the current padrino admin ORM translations to look something like this: https://gist.github.com/lastcanal/5281134. That way in the future if someone has a more specific translation for 'not_decimal' then it can be included.

Contributor

lastcanal commented Mar 31, 2013

I'm in favor or dropping Padrino::Ohm::Validations. We could include a rake ohm:translations task for building i18n translation documents with Ohm's error keys.

It would certainly be possible to map all of the current padrino admin ORM translations to look something like this: https://gist.github.com/lastcanal/5281134. That way in the future if someone has a more specific translation for 'not_decimal' then it can be included.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Apr 2, 2013

Contributor

Good, let's drop validations then.

I'm in favour of adding the translations to Padrino's I18n files directly (we're already mapping AR/AM, so it's fine if we do Ohm too).

The only thing we need to look after is Ohm errors coming like {:email=>[:not_present, :not_email]} which was sort of addressed by https://github.com/padrino/padrino-framework/pull/1196/files#L1R83. Is that what you mean by the rake task?

Contributor

dariocravero commented Apr 2, 2013

Good, let's drop validations then.

I'm in favour of adding the translations to Padrino's I18n files directly (we're already mapping AR/AM, so it's fine if we do Ohm too).

The only thing we need to look after is Ohm errors coming like {:email=>[:not_present, :not_email]} which was sort of addressed by https://github.com/padrino/padrino-framework/pull/1196/files#L1R83. Is that what you mean by the rake task?

@lastcanal

This comment has been minimized.

Show comment
Hide comment
@lastcanal

lastcanal Apr 6, 2013

Contributor

I now have Ohm model errors translating without the extra validations module. I've updated Helpers::FormHelper#error_message_on to handle Ohm's errors. I could't figure out an 'admin only' way to get this to work.

I have also included translations for Ohm's errors in the admin orm locales. Here is the script I used to do the conversion. I haven't included the script in the source tree.

The rake task I was referring to would be for building translation yaml files from Ohm models. Similar to how they are being generated for ActiveRecord with this task.

Currently this patch will only look for Ohm errors in *.ohm.errors.messages.*. In the coming weeks I am hoping to set up translation fallbacks similarly to how ActiveModel does it here. Maybe that would be best done in an I18n gem for Ohm?

Contributor

lastcanal commented Apr 6, 2013

I now have Ohm model errors translating without the extra validations module. I've updated Helpers::FormHelper#error_message_on to handle Ohm's errors. I could't figure out an 'admin only' way to get this to work.

I have also included translations for Ohm's errors in the admin orm locales. Here is the script I used to do the conversion. I haven't included the script in the source tree.

The rake task I was referring to would be for building translation yaml files from Ohm models. Similar to how they are being generated for ActiveRecord with this task.

Currently this patch will only look for Ohm errors in *.ohm.errors.messages.*. In the coming weeks I am hoping to set up translation fallbacks similarly to how ActiveModel does it here. Maybe that would be best done in an I18n gem for Ohm?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 9, 2013

Member

This looks good I think based on #1148 so merging this in.

Member

nesquena commented Apr 9, 2013

This looks good I think based on #1148 so merging this in.

nesquena added a commit that referenced this pull request Apr 9, 2013

Merge pull request #1196 from lastcanal/ohm_activemodel_validations
Remove Ohm monkey patch and include Padrino::Ohm::Validator

@nesquena nesquena merged commit 4f679c7 into padrino:master Apr 9, 2013

1 check passed

default The Travis build passed
Details
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 9, 2013

Member

Thanks @lastcanal for your help with this.

Member

nesquena commented Apr 9, 2013

Thanks @lastcanal for your help with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment