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

Don't store translations for locales not set as available with I18n#available_locales= #261

Conversation

chipairon
Copy link
Contributor

If available locales are set explicitly, that means we don't want to work with other locales. This commit puts a check on Simple#store_translation to check if @@available_locales has been set explicitly, and in that case, does not store the data.

Right now, this translations are kept in memory and cached, but never used.

If the value of 'available_locales' is not set explicitly, it works as before, storing translations in any locale.

@radar
Copy link
Collaborator

radar commented Nov 16, 2016

I am not sure what bug this is fixing. Do you have a simple test case that I can use to reproduce it? I've looked at the test included in this PR and it doesn't make sense as a way to reproduce this bug for me.

@radar radar added the waiting label Nov 16, 2016
@chipairon
Copy link
Contributor Author

Frameworks/Apps using this gem usually include in the load path for translations all the translation files that they find:

Rails does: https://github.com/rails/rails/blob/fe1f4b2ad56f010a4e9b93d547d63a15953d9dc2/railties/lib/rails/engine.rb#L587

Some projects use the default translations provided by other gems, and they also add all locale files to the load path. For instance:
https://github.com/svenfuchs/rails-i18n/blob/4223a0010e81242c69dfb5cde7b314f79cea2f0b/lib/rails_i18n/railtie.rb#L21

With all those files in the "l18n.load_path", on initialization, the simple backend will parse and load in memory ALL translations found in all the files:
https://github.com/svenfuchs/i18n/blob/ae84ad33e1f48a19f0007fdadb66a46e6e5a1d9e/lib/i18n/backend/base.rb#L14

This pull request tries to only load translations for the specified available locales, even if there are translations in the load path for a lot more languages. They are being kept in memory and never used.

@@ -29,6 +29,11 @@ def initialized?
# translations will be overwritten by new ones only at the deepest
# level of the hash.
def store_translations(locale, data, options = {})
if I18n.available_locales_initialized? &&
I18n.available_locales.include?(locale.to_sym) == false &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could probably reduce this to just !I18n.available_locales.map(&:to_s).include?(locale.to_s)

@radar
Copy link
Collaborator

radar commented Nov 16, 2016

Ok, that makes sense to me. Please update this PR against the current master branch and fix the code that I commented on.

@radar
Copy link
Collaborator

radar commented Nov 8, 2017

Superseded by #391.

@radar radar closed this Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants