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

Adding German (Switzerland), English (UK) locales #1916

Merged
merged 1 commit into from Apr 5, 2013

Conversation

psy-q
Copy link

@psy-q psy-q commented Feb 13, 2013

And I've fixed some missing hyphens in the German one as well.

The idea behind the en-GB locale is to make apps work that set I18n.locale to en-GB. The same could be done for en-CA and other English locales, even if no words currently diverge betweeen those.

Otherwise, if these locales are missing, applications setting them will find "translation missing" errors in their ActiveAdmin panels.

The de-CH locale file is there because German in Switzerland does not use ß, and the de.yml file contains the word "Größer".

No failing tests here (I would hope so!) :)

@seanlinsley
Copy link
Contributor

Instead of including an entire file for en-GB, why not specify a fallback?

# in your application config
config.i18n.fallbacks = {'en-GB' => 'en'}

There are more options on stack overflow.

For de-CH why not create have this file:

de-CH:
  greater_than: "Grösser als"

And set up another fallback:

config.i18n.fallbacks = {'en-GB' => 'en', 'de-CH' => 'de'}

@psy-q
Copy link
Author

psy-q commented Feb 14, 2013

Fantastic idea, I'll give it a try. I just emulated what the rails-i18n guys themselves did, they often have the entire file even for identical languages. Compare their en.yml and en-CA.yml for example. If fallbacks are supported in all Rails 3 versions, I'll go down that path.

@seanlinsley
Copy link
Contributor

It shouldn't be a Rails version dependency, it should be the version of the I18n gem you're using.

Actually, the stack overflow link I mentioned has lots of outdated solutions, so the Rails.config setting is probably fairly new.

@psy-q
Copy link
Author

psy-q commented Feb 14, 2013

I notice some problems with providing fallbacks:

  • If we do the fallback configuration in the ActiveAdmin gem, it might conflict with a fallback configuration an app that uses ActiveAdmin has already done. For example, an English (New Zealand) user might not want to fall back on English (US) but instead on English (UK), we can't hardcode a fallback for en-NZ to en if en is in reality en-US.
  • If we don't do the fallback configuration inside ActiveAdmin, users who don't create a fallback configuration will end up with "de-CH:foo:translation missing" messages all over their application.

Which one is better? I realize that only providing those strings that are actually different from a fallback is more elegant and saves space, but can we address those two problems inside the gem?

@seanlinsley
Copy link
Contributor

Ultimately, it's the app developer's responsibility to provide translations.

While in development you'll get an exception if a translation is missing, in production it'll use I18n.default_locale. Hence the default line in config/environments/production.rb

# Enable locale fallbacks for I18n (makes lookups for any locale fall back to
# the I18n.default_locale when a translation can not be found)
config.i18n.fallbacks = true

If an app developer doesn't like our default locales, they can wipe them out by calling

config.i18n.fallbacks.clear

Note that any Active Admin translations can be easily overridden by an app dev:

# config/locales/en.yml
en:
  active_admin:
    filter: "Foobar"

Since this is all very configurable, I think it's no problem to set defaults in Active Admin. What do you think?

@psy-q
Copy link
Author

psy-q commented Feb 16, 2013

Sounds good! Where in the ActiveAdmin gem can we modify config? I never wrote a gem or engine before, so I'm not sure how you can influence the Rails app's config that the gem or engine runs in. I'm happy to figure this out and add it in the required areas, but I'd need some guidance :)

Also, if anyone ever provides a translation for Portuguese (not Brazilian Portuguese), we'd probably need to do the same fallback boogie there.

@psy-q
Copy link
Author

psy-q commented Feb 20, 2013

I've tried to use the fallbacks, but they don't seem to work.

Does nothing, the "translation missing" errors still come up in ActiveAdmin. Are you sure fallbacks in this form are still supported in Rails i18n? They are not even mentioned in the [http://guides.rubyonrails.org/i18n.html](Rails I18n guide), and at least the combinations I've tried (gathered from random forum postings and stackoverflow) did not work. I've tried:

config.i18n.fallbacks = true

Should fall back to the default locale, does nothing ("translation missing").

config.i18n.fallbacks = {'en-GB'=> 'en', 'de-CH' => 'de'}

Should fall back on each of the configured locales listed in the values if the one in the matching key isn't found. Does nothing ("translation missing").

config.i18n.fallbacks = true

Should fall back to config.i18n.default_locale when a translation isn't found, but does nothing. This is with i18n 0.6.1 on Rails 3.2.12. The project I am seeing this in is at https://github.com/zhdk/madek/tree/next. I will create a separate, empty Rails project to verify this, though.

But I thought again about the problem in general: I think we should not require our users to create a fallback configuration that must match the locale files we create for ActiveAdmin in order for their ActiveAdmin interface to be localized properly. I think we should include the complete .yml files, like the other projects do.

Otherwise we create a dependency on the user's app to include the proper configuration and we would have to document this behavior in the I18n section of the ActiveAdmin site, and anyone who misses this section will end up with "translation missing" all over their application even though we could include a perfectly complete translation with the gem.

What does everyone think, wouldn't it be acceptable to include a few more bytes of information in order to reduce this dependency?

@psy-q
Copy link
Author

psy-q commented Feb 20, 2013

It seems the issue with fallbacks not working is only when using https://github.com/grosser/gettext_i18n_rails. But I presume it applies to any translation backend that does not support fallbacks (?).

When I tried the fallbacks on an empty Rails project, they worked.

I think this is an additional reason to include the complete files for each language, that way ActiveAdmin users can pick any translation backend, even if it doesn't have the same kind of fallback configuration as Rails' own.

@macfanatic
Copy link
Contributor

@psy-q Where did you post the fallback code in AA? I would expect it would need to go inside an initializer block in the engine itself, such as:

module ActiveAdmin
  class Engine < Rails::Engine
    if Rails.version > "3.1"

      initializer "ActiveAdmin precompile hook" do |app|
        app.config.assets.precompile += %w(active_admin.js active_admin.css active_admin/print.css)
      end

      # your new initializer code
      initializer "ActiveAdmin i18n fallbacks" do |app|
        app.config.i18n.fallbacks = {'en-GB'=> 'en', 'de-CH' => 'de'}
        app.config.i18n.fallbacks = true
      end
    end

  end
end

If that does work, definitely work on adding some documentation for this great new info :)

@psy-q
Copy link
Author

psy-q commented Mar 5, 2013

Thanks for the hint, I'll try. I'm quite sure this won't work for I18n backends that don't support that fallback mechanism, like e.g. gettext, and so we should perhaps not use it? After all, this effort is to save a few bytes of text by introducing new branches of code (that won't even work for all AA users), is that really smart? I'd prefer if we could include the entire yml file for each language :(

I'll give it a try with gettext.

@psy-q
Copy link
Author

psy-q commented Mar 8, 2013

I'm afraid we will have to include the entire language file. I've tested your suggestion with gettext, and it doesn't work. I presume that gettext does not support fallback locales.

Here are the results using a full en-GB.yml file and a sparse (containing only "Grösser als" instead of "Größer als") de-CH.yml:

1.9.3-p194 :005 > I18n.locale = "en-GB"
 => "en-GB" 
1.9.3-p194 :006 > I18n.t("active_admin.greater_than")
 => "Greater Than" 
1.9.3-p194 :009 > I18n.locale = "de-CH"
 => "de-CH" 
1.9.3-p194 :010 > I18n.t("active_admin.greater_than")
 => "Grösser als" 
1.9.3-p194 :012 > I18n.t("active_admin.edit")
 => "translation missing: de-CH.active_admin.edit" 

It might work if using only Rails' simple I18n, but for anyone using a different translation mechanism, this isn't a solution. I will revert my last commit.

I'd be really happy if the pull request could be accepted like this. I think we really tried to save a few bytes here, but it doesn't appear to be possible.

@seanlinsley
Copy link
Contributor

Our Travis builds on master are now green (as of 166bc46). Please rebase your PR on the current master:

# Expectations: "upstream" is gregbell's GitHub repo and "origin" is your fork

# Rebase your fork's master branch with the latest upstream changes
git checkout master
git pull --rebase upstream master
git push origin master

# Rebase your feature branch with the latest upstream changes
git checkout your_feature_branch
git pull --rebase upstream master
git push origin your_feature_branch # note that you may need to use -f

@psy-q
Copy link
Author

psy-q commented Mar 19, 2013

I hope I rebased correctly, my commits are now done against the current (yesterday's) state of master.

@seanlinsley
Copy link
Contributor

Yep, looks like it. I'll need to do a bit more research on this stuff (your recent comments) before pulling this in, though.

@seanlinsley
Copy link
Contributor

@psy-q can you squash all these commits into a single commit with a suitable commit message for the PR?

@seanlinsley
Copy link
Contributor

ping!

@psy-q
Copy link
Author

psy-q commented Apr 2, 2013

Sorry, was away over the easter weekend. I'll try a squash now.

@psy-q
Copy link
Author

psy-q commented Apr 2, 2013

I think I've cleaned it up now. Is this better or should I amend the commit message to not include the squash details?

@seanlinsley
Copy link
Contributor

Yeah, I'd appreciate if you could remove the squash details and only leave stuff in the commit message that someone reading your commit in the future would care about.

@psy-q
Copy link
Author

psy-q commented Apr 3, 2013

OK, done, I hope!

@seanlinsley
Copy link
Contributor

Yep, the commit message looks good. Except now there's an unneeded merge commit. If you were trying to rebase your PR on master, please follow these instructions instead.

@psy-q
Copy link
Author

psy-q commented Apr 5, 2013

I think now I've managed. Sheesh!

@seanlinsley
Copy link
Contributor

Yep. Merging :)

seanlinsley added a commit that referenced this pull request Apr 5, 2013
Adding German (Switzerland), English (UK) locales
@seanlinsley seanlinsley merged commit a474103 into activeadmin:master Apr 5, 2013
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.

None yet

3 participants