Should we separate Locale settings from translations?

[Luca Guidi]

Hi all,
The new exception handling turns the lights on a new issue: the
separation between locale settings and translations.

MissingTranslationData make sense for:
:'bogus'.t # => "translation missing: en-US, bogus"

but not for:
:'date.formats.default'.t #=> "translation missing: en-US, date,
formats, default"

I suggest to namespace locale settings with :locale key, and if one of
those settings is missing we should raise a MissingLocaleData exception.

This because, a missing translation is quite different from a missing
locale setting.

Look at:
http://github.com/svenfuchs/rails/tree/6982acb0793fb6e59f52cab4062344a88e3691ce/actionmailer/lib/action_mailer/base.rb#L352

def charset(charset)
@charset ||= charset || :'charset'.t || @@default_charset
end

With the current i18n implementation, if the charset wasn't defined, the
second lookup fails, returning the following string:
"translation missing: en-US, charset"

It makes impossible to reach the third condition (@@default_charset),
unless we catch the failure string.

With the new i18n implementation it could be:
def charset(charset = nil)
@charset ||= charset || :'locale.charset'.t rescue nil ||
@@default_charset
end

What do you think about? Of course, I attached a patch for this.

Luca
--
blog: www.lucaguidi.com
Pro-Netics: www.pro-netics.com
Sourcesense - making sense of Open Source: www.sourcesense.com

[Sven Fuchs]

Hi Luca,

for some weird reason I have problems with applying these patches. It
always tells me that the patch is invalid (at line 13 in this case).

Anyways.

I can see why you'd want to introduce a namespace for this, but did
you notice that you can just do

@charset ||= charset || I18n.t(:charset, :default => @@default_charset)

instead?

The same happens in ActiveRecord validations #full_messages where it
tries to lookup a db column name and uses #humanize_attribute_name as
a default.

This (translated column names) is also an example where the
translation would certainly not go into a :locale namespace even if
we'd invent it.

> From 99c0492b476ff97f041bfcf76d3306d5e7545dec Mon Sep 17 00:00:00 2001
> From: Luca Guidi <guidi...@gmail.com>
> Date: Wed, 2 Jul 2008 15:37:55 +0200
> Subject: [PATCH] Namespaced locale settings, in order to separate
> from translations
>
> ---
> lib/i18n.rb | 1 +
> lib/i18n/backend/simple.rb | 20 +++++++++++++-------
> test/i18n_test.rb | 28 ++++++++++++++++++----------
> test/simple_backend_test.rb | 40 ++++++++++++++++++++
> +-------------------
> 4 files changed, 53 insertions(+), 36 deletions(-)
>
> diff --git a/lib/i18n.rb b/lib/i18n.rb
> index 9fcb53f..011e93b 100755
> --- a/lib/i18n.rb
> +++ b/lib/i18n.rb
> @@ -12,6 +12,7 @@ module I18n
> class ArgumentError < ::ArgumentError; end
> class InvalidLocale < ArgumentError; end
> class MissingTranslationData < ArgumentError; end
> + class MissingLocaleData < ArgumentError; end
> class InvalidPluralizationData < ArgumentError; end
> class MissingInterpolationArgument < ArgumentError; end
> class ReservedInterpolationKey < ArgumentError; end
> diff --git a/lib/i18n/backend/simple.rb b/lib/i18n/backend/simple.rb
> index 9f0461f..c76b684 100644
> --- a/lib/i18n/backend/simple.rb
> +++ b/lib/i18n/backend/simple.rb
> @@ -30,7 +30,13 @@ module I18n
> options.delete(:default)
> values = options.reject{|name, value| reserved.include?
> name }
>
> - entry = lookup(locale, key, scope) || default(locale,
> default, options) || raise(I18n::MissingTranslationData,
> "translation data missing for #{normalize_keys(locale, key,
> scope).inspect}")
> + entry = lookup(locale, key, scope) || default(locale,
> default, options) || begin
> + if /^locale/.match key.to_s
> + raise(I18n::MissingLocaleData, "locale data missing
> for #{normalize_keys(locale, key, scope).inspect}")
> + else
> + raise(I18n::MissingTranslationData, "translation data
> missing for #{normalize_keys(locale, key, scope).inspect}")
> + end
> + end
> entry = pluralize entry, count
> entry = interpolate entry, values
> entry
> @@ -43,16 +49,16 @@ module I18n
> raise ArgumentError, "Object must be a Date, DateTime or
> Time object. #{object.inspect} given." unless object.respond_to?
> (:strftime)
>
> type = object.respond_to?(:sec) ? 'time' : 'date'
> - formats = :"#{type}.formats".t locale
> + formats = :"locale.#{type}.formats".t locale
> format = formats[format.to_sym] if formats &&
> formats[format.to_sym]
> # TODO raise exception unless format found?
> format = format.to_s.dup
>
> - format.gsub!(/%a/, :"date.abbr_day_names".t(locale)
> [object.wday])
> - format.gsub!(/%A/, :"date.day_names".t(locale)
> [object.wday])
> - format.gsub!(/%b/, :"date.abbr_month_names".t(locale)
> [object.mon])
> - format.gsub!(/%B/, :"date.month_names".t(locale)
> [object.mon])
> - format.gsub!(/%p/, :"time.#{object.hour <
> 12 ? :am : :pm}".t(locale)) if object.respond_to? :hour
> + format.gsub!(/%a/, :"locale.date.abbr_day_names".t(locale)
> [object.wday])
> + format.gsub!(/%A/, :"locale.date.day_names".t(locale)
> [object.wday])
> + format.gsub!(/
> %b/, :"locale.date.abbr_month_names".t(locale)[object.mon])
> + format.gsub!(/%B/, :"locale.date.month_names".t(locale)
> [object.mon])
> + format.gsub!(/%p/, :"locale.time.#{object.hour <
> 12 ? :am : :pm}".t(locale)) if object.respond_to? :hour
> object.strftime(format)
> end
>
> diff --git a/test/i18n_test.rb b/test/i18n_test.rb
> index de7351b..a89732b 100644
> --- a/test/i18n_test.rb
> +++ b/test/i18n_test.rb
> @@ -14,10 +14,12 @@ end
> class I18nTest < Test::Unit::TestCase
> def setup
> I18n.backend.store_translations :'en-US', {
> - :currency => {
> - :format => {
> - :separator => '.',
> - :delimiter => ',',
> + :locale => {
> + :currency => {
> + :format => {
> + :separator => '.',
> + :delimiter => ',',
> + }
> }
> }
> }
> @@ -81,27 +83,27 @@ class I18nTest < Test::Unit::TestCase
> end
>
> def test_translate_on_nested_symbol_keys_works
> - assert_equal ".", I18n.t(:'currency.format.separator', 'en-US')
> + assert_equal ".", I18n.t(:'locale.currency.format.separator',
> 'en-US')
> end
>
> def test_translate_with_nested_string_keys_works
> - assert_equal ".", I18n.t('currency.format.separator', 'en-US')
> + assert_equal ".", I18n.t('locale.currency.format.separator',
> 'en-US')
> end
>
> def test_translate_with_array_as_scope_works
> - assert_equal ".", I18n.t(:separator, 'en-US', :scope =>
> ['currency.format'])
> + assert_equal ".", I18n.t(:separator, 'en-US', :scope =>
> ['locale.currency.format'])
> end
>
> def
> test_translate_with_array_containing_dot_separated_strings_as_scope_works
> - assert_equal ".", I18n.t(:separator, 'en-US', :scope =>
> ['currency.format'])
> + assert_equal ".", I18n.t(:separator, 'en-US', :scope =>
> ['locale.currency.format'])
> end
>
> def test_translate_with_key_array_and_dot_separated_scope_works
> - assert_equal [".", ","], I18n.t(%w(separator delimiter), 'en-
> US', :scope => 'currency.format')
> + assert_equal [".", ","], I18n.t(%w(separator delimiter), 'en-
> US', :scope => 'locale.currency.format')
> end
>
> def test_translate_with_dot_separated_key_array_and_scope_works
> - assert_equal [".", ","], I18n.t(%w(format.separator
> format.delimiter), 'en-US', :scope => 'currency')
> + assert_equal [".", ","], I18n.t(%w(format.separator
> format.delimiter), 'en-US', :scope => 'locale.currency')
> end
>
> def test_translate_with_options_using_scope_works
> @@ -119,6 +121,12 @@ class I18nTest < Test::Unit::TestCase
> assert_equal "translation missing: en-US, bogus", I18n.t(:bogus)
> end
>
> + def test_translate_missing_locale_setting_should_raise_exception
> + assert_raise I18n::MissingLocaleData do
> + I18n.t :'locale.missing_setting'
> + end
> + end
> +
> def test_localize_nil_raises_argument_error
> assert_raises(I18n::ArgumentError) { I18n.l nil }
> end
> diff --git a/test/simple_backend_test.rb b/test/simple_backend_test.rb
> index 4c1b8c2..7b4d4ad 100644
> --- a/test/simple_backend_test.rb
> +++ b/test/simple_backend_test.rb
> @@ -15,26 +15,28 @@ module I18nSimpleBackendTestSetup
>
> def add_datetime_translations
> @backend.store_translations :'de-DE', {
> - :date => {
> - :formats => {
> - :default => "%d.%m.%Y",
> - :short => "%d. %b",
> - :long => "%d. %B %Y",
> + :locale => {
> + :date => {
> + :formats => {
> + :default => "%d.%m.%Y",
> + :short => "%d. %b",
> + :long => "%d. %B %Y",
> + },
> + :day_names => %w(Sonntag Montag Dienstag Mittwoch
> Donnerstag Freitag Samstag),
> + :abbr_day_names => %w(So Mo Di Mi Do Fr Sa),
> + :month_names => %w(Januar Februar März April Mai Juni
> Juli August September Oktober November Dezember).unshift(nil),
> + :abbr_month_names => %w(Jan Feb Mar Apr Mai Jun Jul Aug
> Sep Okt Nov Dez).unshift(nil),
> + :order => [:day, :month, :year]
> },
> - :day_names => %w(Sonntag Montag Dienstag Mittwoch
> Donnerstag Freitag Samstag),
> - :abbr_day_names => %w(So Mo Di Mi Do Fr Sa),
> - :month_names => %w(Januar Februar März April Mai Juni Juli
> August September Oktober November Dezember).unshift(nil),
> - :abbr_month_names => %w(Jan Feb Mar Apr Mai Jun Jul Aug Sep
> Okt Nov Dez).unshift(nil),
> - :order => [:day, :month, :year]
> - },
> - :time => {
> - :formats => {
> - :default => "%a, %d. %b %Y %H:%M:%S %z",
> - :short => "%d. %b %H:%M",
> - :long => "%d. %B %Y %H:%M",
> - },
> - :am => 'am',
> - :pm => 'pm'
> + :time => {
> + :formats => {
> + :default => "%a, %d. %b %Y %H:%M:%S %z",
> + :short => "%d. %b %H:%M",
> + :long => "%d. %B %Y %H:%M",
> + },
> + :am => 'am',
> + :pm => 'pm'
> + }
> },
> :datetime => {
> :distance_in_words => {
> --
> 1.5.4.5
>

--
sven fuchs sven...@artweb-design.de
artweb design http://www.artweb-design.de
grünberger 65 + 49 (0) 30 - 47 98 69 96 (phone)
d-10245 berlin + 49 (0) 171 - 35 20 38 4 (mobile)

[Luca Guidi]

Sven Fuchs wrote:
> @charset ||= charset || I18n.t(:charset, :default => @@default_charset)

Yes, of course, I always forget about :default :P

My proposal for the :locale namespace, it's a way to separate
translations from locale settings, because they are conceptually different.

I think a missing locale setting (i.e. charset) is more important than
a missing translation, so they should have a different handling. In the
patch I introduced MissingLocaleData exception for this kind of
situation. In this case the namespace helps to distinguish from settings
and translations.

Best,