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

Fix inconsistency with other I18n backends #86

Conversation

gingermusketeer
Copy link
Contributor

I18n::Backend::Simple supports returning all keys by invoking I18n.t('.'). This adds support for this functionality so that it is consistent with the simple backend.

I ran into this problem when using different backends across environments in rails.

Hopefully the changes I have made make sense!

Why: So that tests can be run in isolation.
Why: So that the ActiveRecord Backend is consistent with the default rails backend.
@@ -51,6 +51,10 @@ def lookup(locale, key, scope = [], options = {})
key = normalize_flat_keys(locale, key, scope, options[:separator])
result = Translation.locale(locale).lookup(key)

if result.empty? && key == '.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we call this before or instead result = Translation.locale(locale).lookup(key)
Maybe smth like

result = if key == '.'
  Translation.locale(locale).all
else
  Translation.locale(locale).lookup(key)
end

This way we won't run two queries for . key

@timfjord
Copy link
Collaborator

@gingermusketeer Thanks for your contribution. I've added one comment

Why: So the database is only hit once for the lookup.
@gingermusketeer
Copy link
Contributor Author

@Timsly Thanks for pointing that out. I have made the change you suggested!

@timfjord timfjord merged commit 76f4e44 into svenfuchs:master Nov 17, 2017
@timfjord
Copy link
Collaborator

Merged! Thanks @gingermusketeer

@gingermusketeer gingermusketeer deleted the fix-inconsistency-with-other-i18n-backends branch November 17, 2017 10:16
@gingermusketeer
Copy link
Contributor Author

Thank you!!

@timfjord
Copy link
Collaborator

Released as 0.2.1

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.

2 participants