Impossible to override I18n translations #1775

Merged
merged 2 commits into from Nov 29, 2012

Conversation

Projects
None yet
3 participants
Contributor

caifara commented Nov 18, 2012

Locale files with translations duplicating the activeadmin keys are ignored.

@pcreux pcreux and 2 others commented on an outdated diff Nov 28, 2012

lib/active_admin.rb
@@ -53,7 +53,7 @@ class Railtie < ::Rails::Railtie
config.after_initialize do
# Add load paths straight to I18n, so engines and application can overwrite it.
require 'active_support/i18n'
- I18n.load_path += Dir[File.expand_path('../active_admin/locales/*.yml', __FILE__)]
+ I18n.load_path.unshift Dir[File.expand_path('../active_admin/locales/*.yml', __FILE__)]
@pcreux

pcreux Nov 28, 2012

Contributor

You want to see your translations taking over the default ones from active admin. Doesn't this prepend active admin default locale to the load path hence give them priority?

@caifara

caifara Nov 28, 2012

Contributor

Hi and thanks for reading this through ...

I did not look into I18n machinery very deeply. What I do know is that the added scenario doesn't work without this change. So the view doesn't use the overridden translation until this line is changed. This could also have something to do with the moment of execution of this line in comparison with the adding of the translations in the rails app.

@pcreux

pcreux Nov 28, 2012

Contributor

Could you try doing I18n.load_path = Dir[] + I18n.load_path instead?

Because:

[1, 2].unshift [3, 4]
# => [[3, 4], 1, 2]

Also, if you can look into the I18n machinery. I'm concerned of side effects that this could have for other people. We run into so many issues when it comes to i18n.

@quark-zju

quark-zju Nov 28, 2012

Contributor

unshift is ok if * is used:

I18n.load_path.unshift *Dir[File.expand_path('../active_admin/locales/*.yml', __FILE__)]

unshift should be faster than +.

Active Support uses both unshift * and + in activesupport-3.2.9/lib/active_support/i18n_railtie.rb

Contributor

quark-zju commented Nov 28, 2012

I encoutered the same problem. Overriding I18n translations should be possible. I think @caifara does things correct and have no side effects.

Besides, it is better to move I18n translations to a separated package like devise-i18n and rails-i18n. It makes sence because I18n translations are optional, they should not be loaded into memory unless user explicitly do so.

Contributor

caifara commented Nov 28, 2012

As far as I can tell and with the suggestions from @quark-zju this should work correctly. I'm running the tests with the splat operator now (I did not add a spec or feature test to make sure the * stays in for now).

(Oh, and consider me a fan of moving I18n translations out of the core package)

Contributor

pcreux commented Nov 29, 2012

Thanks @quark-zju for confirming this. I'm merging this PR in.

@caifara do you want to work on moving I18n out?

@pcreux pcreux added a commit that referenced this pull request Nov 29, 2012

@pcreux pcreux Merge pull request #1775 from caifara/1775-override-i18n-translations
Impossible to override I18n translations
ba7e63a

@pcreux pcreux merged commit ba7e63a into activeadmin:master Nov 29, 2012

Contributor

caifara commented Nov 29, 2012

@pcreux Programming is only part of one of my jobs at the moment. However, I would still like to consider it.

How should the end result look like in your opinion? (we'd better start a new issue I suppose)

pcreux referenced this pull request Dec 6, 2012

Closed

I18n error! #1803

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