This unfortunately broke with the recent babel upgrade. #2757
Conversation
…grade. Make sure we use Locale.parse, this also adds a test that makes sure we now can resolve all language codes that reside in AMO_LANGUAGES. Fixes #2753
EnTeQuAk
force-pushed
the
bugfix/2753-fix-locale-parsing
branch
from
dda3cb7
to
010ba0c
Compare
Current coverage is 93.12%@@ master mozilla/addons-server#2757 diff @@
==========================================
Files 474 474
Lines 61617 61712 +95
Methods 0 0
Messages 0 0
Branches 4374 4399 +25
==========================================
+ Hits 57383 57465 +82
- Misses 3486 3497 +11
- Partials 748 750 +2
|
| @pytest.mark.parametrize('lang', settings.AMO_LANGUAGES) | ||
| def test_get_locale_from_lang(lang): | ||
| """Make sure all languages in settings.AMO_LANGUAGES can be resolved.""" | ||
| get_locale_from_lang(lang) |
There was a problem hiding this comment.
Should we check the result as well while we're at it ?
There was a problem hiding this comment.
Hmm, I'll see if I can find out how. I don't want to have another
mapping of AMO_LANGUAGES to Locale instances but maybe that's not a
bad idea...
On Thu, May 26, 2016, at 10:04 AM, Mathieu Pillard wrote:
In src/olympia/amo/tests/test_utils_.py[1]:
@@ -172,3 +174,9 @@ def bar(self): > del foo.bar > assert
foo.bar == 'original value' > assert callme.call_count == 1 > +
+@pytest.mark.parametrize('lang', settings.AMO_LANGUAGES) >
+def test_get_locale_from_lang(lang): > + """Make sure all
languages in settings.AMO_LANGUAGES can be resolved.""" > +
get_locale_from_lang(lang)Should we check the result as well while we're at it ?
—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub[2]
Links:
There was a problem hiding this comment.
Maybe just checking that it's a a Locale instance ?
There was a problem hiding this comment.
And check the country, checking the territory is overkill then I think.
I'll update the PR
On Thu, May 26, 2016, at 10:36 AM, Mathieu Pillard wrote:
In src/olympia/amo/tests/test_utils_.py[1]:
@@ -172,3 +174,9 @@ def bar(self): > del foo.bar > assert
foo.bar == 'original value' > assert callme.call_count == 1 > +
+@pytest.mark.parametrize('lang', settings.AMO_LANGUAGES) >
+def test_get_locale_from_lang(lang): > + """Make sure all
languages in settings.AMO_LANGUAGES can be resolved.""" > +
get_locale_from_lang(lang)Maybe just checking that it's a a Locale instance ?
—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub[2]
Links:
| locale = get_locale_from_lang(lang) | ||
|
|
||
| assert isinstance(locale, Locale) | ||
| assert locale.language == lang[:2] |
There was a problem hiding this comment.
This did not pass without to the changes from this PR... even with the previous babel version (locale.language returns the language with underscore and territory for all "long" languages, even en-US).
I'm not sure what to make of that. Does it mean that get_locale_from_lang has always been broken, but the new babel started to make the issue more visible ?
There was a problem hiding this comment.
The thing is, babel expects the language code to be encoded as en_US, not en-US which is why we use django.translation.to_locale in get_locale_from_lang to convert it.
Imho get_locale_from_lang has always been broken and should have used Locale.parse, especially for zh-CN and zh-TW since those aren't the actual locale codes but shortcuts for zh-Hans-CN and only Locale.parse understands that. Well, in newer versions of babel at least, it appears older versions supported those as an argument to Locale(...).
Does that answer your question? 😆
There was a problem hiding this comment.
Somewhat. My concern is that the Locale object we return is completely different from before for a lot of languages (as shown by the difference in the value returned by locale.language), so I wonder if it has consequences later down the road.
There was a problem hiding this comment.
get_locale_from_lang is used exactly two times, in our amo.helpers and here it's passed directly to babel. And the other one is in stats.models and passes it also directly to babel.
I think we're safe here. We only pass the locale forward to babel and don't use it ourselfs. It could fix some things, e.g formattings or make them more correct maybe - which I doubt - but that's the only thing I can imagine.
|
r+ |
Make sure we use Locale.parse, this also adds a test that makes
sure we now can resolve all language codes that reside
in AMO_LANGUAGES.
Fixes mozilla/addons#3150