Patch human_attribute_name in ActiveModel::Translation instead of ActiveRecord::Base #17

Closed
nikosd opened this Issue Feb 8, 2011 · 13 comments

Comments

Projects
None yet
3 participants
Contributor

nikosd commented Feb 8, 2011

ActiveRecord::Base#human_attribute_name has been removed from Rails 3 and instead ActiveModel::Translation#human_attribute_name has taken it's place.

Maybe this is what https://github.com/grosser/gettext_i18n_rails/blob/master/lib/gettext_i18n_rails/active_record.rb#L3 should patch.

P.S.: I will try to send 1 or 2 pull requests with this and one more issue fixed if I find some time till the end of the week

Owner

grosser commented Feb 9, 2011

if you can give me a failing spec i can do the patching :)
were using it on one rather small rails 3 project and did not run into this so far...

Contributor

nikosd commented Feb 9, 2011

There's no problem as long as you use ActiveRecord::Base but it doesn't integrate with other ORMs that inherit behaviour automatically from ActiveModel. So basically gettext_i18n_rails overrides ActiveModel's human_attribute_name by adding it to ActiveRecord::Base objects.

I will try to see whether I can spend some trying to make this both rails 3 and rails 2 compliant in a non-ugly way.

Owner

grosser commented Feb 10, 2011

ahh now I get it, overread the ActiveModel part :>
maybe something like
translation_class = (rails3 ? ActiveModel::Base : AciveRecord::Base )
translation_class.class_eval do .... end

Contributor

nikosd commented Feb 10, 2011

yeah something like that - i'm not sure though that the call to "rails3" is the proper way to do it. I was thinking something like "defined?(ActiveModel::Translation)" but I'm not sure whether the module has to be monkey-patched or the class itself. I'll have to get a small level up in ruby regarding Classes, Modules, inheritance and monkey-patching.

Also, do you think it would it make sense to add a Rails 3 specific test for ActiveModel next to ActiveRecord?

Owner

grosser commented Feb 10, 2011

defined?(ActiveModel::Translation) sounds good.
yep you should add it, specs run in rails2/rails3 mode on each run, so surround it with something like if rails3 ... end

Contributor

nikosd commented Feb 12, 2011

Status update : I tried to do it without breaking backwards compatibility but I didn't like the hacks and the duplicate code. So, here's a first take on Rails 3 integration through ActiveModel without keeping backwars compatibility : https://github.com/nikosd/gettext_i18n_rails/tree/rails3

Open to suggestions...

P.S.: If you don't like the idea of breaking Rails 2 let me know so I can start using my own "fork" for my projects

Owner

grosser commented Feb 12, 2011

I think ill have to just branch of to rails2 and keep master for rails3, could double the pull/merge work, but the code/tests will get simpler...

Ill give your active_model goodness a try on our rails3 project and see if it breaks anything, then do the merging/cleanup

Contributor

nikosd commented Feb 12, 2011

Nice, I will also try it on monday on ours but the problem with ours is that it hasn't yet been translated so I can't be sure 100% that everything is working as it should.

Owner

grosser commented Feb 13, 2011

Just used https://github.com/grosser/gettext_i18n_rails_example/tree/rails3 to test it and found a bug:
if you translate a rails message like "activerecord.errors.models.car.attributes.model.blank"
it would normally translate it via gettext, but with the update it no longer does.

The normal translations seem to work fine, i think ill add an example for nested attributes too, so we can also see it in action :>

Contributor

nikosd commented Feb 13, 2011

Noted - I will check it out also

Contributor

retoo commented Oct 5, 2011

Hi Nokos, any news on that? I think had the same problem with active resources.

Contributor

nikosd commented Oct 15, 2011

Hey @retoo. I have this tab open for more than 1 month but still haven't found the time to actually work on it. Pretty soon though I will because we will need this functionality.

Owner

grosser commented Jun 3, 2012

This should be solved

@grosser grosser closed this Jun 3, 2012

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