Skip to content
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

lastcanal
Copy link
Contributor

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
Copy link

Thanks @lastcanal! Thoughts @skade @ujifgc?

@lastcanal
Copy link
Contributor Author

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
Copy link

@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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link
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
Remove Ohm monkey patch and include Padrino::Ohm::Validator
@nesquena nesquena merged commit 4f679c7 into padrino:master Apr 9, 2013
@nesquena
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants