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

Impossible to override I18n translations #1775

Merged
merged 2 commits into from
Nov 29, 2012

Conversation

caifara
Copy link
Contributor

@caifara caifara commented Nov 18, 2012

Locale files with translations duplicating the activeadmin keys are ignored.

@@ -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__)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@quark-zju
Copy link
Contributor

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.

@caifara
Copy link
Contributor Author

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)

@pcreux
Copy link
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 added a commit that referenced this pull request Nov 29, 2012
@pcreux pcreux merged commit ba7e63a into activeadmin:master Nov 29, 2012
@caifara
Copy link
Contributor Author

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 pcreux mentioned this pull request Dec 6, 2012
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.

3 participants