Skip to content

Loading…

Fix inflector to respect default locale. #10158

Merged
merged 1 commit into from

9 participants

@steveklabnik
Ruby on Rails member

The inflector was made aware of locales in 7db0b07,
but it defaulted to :en. That should actually be our default
locale instead.

Fixes #10125

I wanted to get some feedback about the tests. Is this the right place, or should I bet testing singularize/pluralize more directly?

/cc @thenickcox

@thenickcox thenickcox Fix inflector to respect default locale.
The inflector was made aware of locales in 7db0b07,
but it defaulted to :en. That should actually be our default
locale instead.

Fixes #10125
8cf88cc
@jeremy
Ruby on Rails member

:+1:

@steveklabnik steveklabnik merged commit fa3ef8e into rails:master
@steveklabnik
Ruby on Rails member

Word, thanks.

@cfabianski cfabianski commented on the diff
activesupport/test/inflector_test.rb
((4 lines not shown))
+
+ def test_inflector_with_default_locale
+ old_locale = I18n.locale
+
+ begin
+ I18n.locale = :de
+
+ ActiveSupport::Inflector.inflections(:de) do |inflect|
+ inflect.irregular 'region', 'regionen'
+ end
+
+ assert_equal('regionen', 'region'.pluralize)
+ assert_equal('region', 'regionen'.singularize)
+ ensure
+ I18n.locale = old_locale
+ end

You might wanna use I18n.with_locale(:de) do ... end for that purpose. (https://github.com/svenfuchs/i18n/blob/master/lib/i18n.rb#L243-L251)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rubys

This needs to be I18n.default_locale. Please see #10163

Ruby on Rails member

Why are we doing this?

An application by a French company may have a default of French for their interface, but all their tables be pluralized in English as we have done always.

Two alternatives, for discussion purposes:

Ruby on Rails member

I would revert this commit. I don't see why we are doing this.

What is the status here?
From my (very limited) point of view pinning Active Record to :en seems to be a good approach.

Ruby on Rails member

This commit was reverted in d716fe0. In addition to Active Record there are more parts of Rails that use the inflector. By now you'll need to pass the locale yourself, or implement wrappers

# config/initializers/core_extensions.rb
String.class_eval do
  def xsingularize(locale=I18n.locale)
    singularize(locale)
  end
end

We are very close to RC1, doubt this will change for Rails 4.

@josevalim
Ruby on Rails member

This issues is not only with Active Record but also routes, rails rendering stack, anywhere that may call pluralize. This definitely needs to be reverted.

@rafaelfranca rafaelfranca added a commit that referenced this pull request
@rafaelfranca rafaelfranca Revert "Merge pull request #10158 from steveklabnik/issue_10125"
This reverts commit fa3ef8e, reversing
changes made to e0af93d.

Reason: Routes, Active Record and the rendering stack should not depend
on the default locale
d716fe0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 10, 2013
  1. @thenickcox @steveklabnik

    Fix inflector to respect default locale.

    thenickcox committed with steveklabnik
    The inflector was made aware of locales in 7db0b07,
    but it defaulted to :en. That should actually be our default
    locale instead.
    
    Fixes #10125
View
4 activesupport/lib/active_support/core_ext/string/inflections.rb
@@ -28,7 +28,7 @@ class String
# 'apple'.pluralize(2) # => "apples"
# 'ley'.pluralize(:es) # => "leyes"
# 'ley'.pluralize(1, :es) # => "ley"
- def pluralize(count = nil, locale = :en)
+ def pluralize(count = nil, locale = I18n.locale)
locale = count if count.is_a?(Symbol)
if count == 1
self
@@ -51,7 +51,7 @@ def pluralize(count = nil, locale = :en)
# 'the blue mailmen'.singularize # => "the blue mailman"
# 'CamelOctopi'.singularize # => "CamelOctopus"
# 'leyes'.singularize(:es) # => "ley"
- def singularize(locale = :en)
+ def singularize(locale = I18n.locale)
ActiveSupport::Inflector.singularize(self, locale)
end
View
17 activesupport/test/inflector_test.rb
@@ -380,6 +380,23 @@ def test_inflector_locality
assert !ActiveSupport::Inflector.inflections.plurals.empty?
assert !ActiveSupport::Inflector.inflections.singulars.empty?
end
+
+ def test_inflector_with_default_locale
+ old_locale = I18n.locale
+
+ begin
+ I18n.locale = :de
+
+ ActiveSupport::Inflector.inflections(:de) do |inflect|
+ inflect.irregular 'region', 'regionen'
+ end
+
+ assert_equal('regionen', 'region'.pluralize)
+ assert_equal('region', 'regionen'.singularize)
+ ensure
+ I18n.locale = old_locale
+ end

You might wanna use I18n.with_locale(:de) do ... end for that purpose. (https://github.com/svenfuchs/i18n/blob/master/lib/i18n.rb#L243-L251)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
def test_clear_all
with_dup do
Something went wrong with that request. Please try again.