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.

Sign up for GitHub

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

Jump to bottom

This unfortunately broke with the recent babel upgrade. #2757

Merged
merged 2 commits into from May 26, 2016
Merged

This unfortunately broke with the recent babel upgrade. #2757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
010ba0c
Fix locale parsing. This unfortunately broke with the recent babel up…
EnTeQuAk May 26, 2016
c685a08
Add test for language and territory
EnTeQuAk May 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/olympia/amo/tests/test_utils_.py
View file
Open in desktop
Expand Up @@ -3,11 +3,14 @@

import mock
import pytest
from babel import Locale
from django.conf import settings

from olympia import amo
from olympia.amo.tests import TestCase, addon_factory
from olympia.amo.utils import (
attach_trans_dict, translations_for_field, walkfiles)
attach_trans_dict, translations_for_field, walkfiles,
get_locale_from_lang)
from olympia.addons.models import Addon
from olympia.versions.models import Version

Expand Down Expand Up @@ -172,3 +175,19 @@ 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."""
locale = get_locale_from_lang(lang)

assert isinstance(locale, Locale)
assert locale.language == lang[:2]
Copy link
Member

@diox diox May 26, 2016 •
edited

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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? 😆

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you're right.


separator = filter(
None, [sep if sep in lang else None for sep in ('-', '_')])

if separator:
territory = lang.split(separator[0])[1]
assert locale.territory == territory
3 changes: 2 additions & 1 deletion src/olympia/amo/utils.py
View file
Open in desktop
Expand Up @@ -597,7 +597,8 @@ def get_locale_from_lang(lang):
# Special fake language can just act like English for formatting and such
if not lang or lang == 'dbg':
lang = 'en'
return Locale(translation.to_locale(lang))

return Locale.parse(translation.to_locale(lang))


class HttpResponseSendFile(http.HttpResponse):
Expand Down