i18n: Handle missing babel locale again #5965
Conversation
unho
force-pushed
the
songhay-fix-again
branch
4 times, most recently
from
b4904af
to
33e0251
Compare
pootle/i18n/dates.py
Outdated
| try: | ||
| return format_timedelta(timedelta, locale=locale) | ||
| except ValueError: | ||
| return format_timedelta(timedelta, locale='en') |
There was a problem hiding this comment.
This needs a test.
This seems like it's following the same pattern from the bits I coded. Can you not refactor that out into a function that devolves to a workable locale.
pootle/i18n/dates.py
Outdated
| try: | ||
| return format_timedelta(timedelta, locale=locale) | ||
| except UnknownLocaleError: | ||
| locale = settings.LANGUAGE_CODE.split("-")[0] |
There was a problem hiding this comment.
Definitely a no no. I'm pretty sure I used something that can give you a valid code.
unho
force-pushed
the
songhay-fix-again
branch
5 times, most recently
from
ddb2c6c
to
3158dc5
Compare
unho
force-pushed
the
songhay-fix-again
branch
2 times, most recently
from
cac82ae
to
82e1a90
Compare
tests/i18n/dates.py
Outdated
| ('en-za', 'en'), | ||
| ('en-us', 'en'), | ||
| # Missing in babel | ||
| ('son', 'en'), |
There was a problem hiding this comment.
Bad one to test, this test will be bust as soon as son comes to babel. Rather us a really broken one that won't ever appear in CLDR
There was a problem hiding this comment.
We are testing here that it catches the error and falls back to default locale.
tests/i18n/dates.py
Outdated
| @pytest.mark.parametrize('language,fallback', [ | ||
| # Normal | ||
| ('af', 'af'), | ||
| ('en-za', 'en'), |
There was a problem hiding this comment.
I'm guessing that this passes by accident rather than by design. For numbers it will fail. So really not sure what we're testing here.
pootle/i18n/formatter.py
Outdated
| for language in [get_language(), settings.LANGUAGE_CODE, 'en-us']: | ||
| def get_locale_formats(locale=None): | ||
| languages = [locale] if locale else [] | ||
| languages += [get_language(), settings.LANGUAGE_CODE, 'en-us', 'en'] |
There was a problem hiding this comment.
Was there a specific reason to fallback to en? Since our app is in en-us it doesn't make much sense to me. And I suspect in CLDR en-us == en so kinda pointless step IIUC and just confuses the matter with no justification in the code or the commit
pootle/i18n/formatter.py
Outdated
| try: | ||
| locale = babel_core.Locale.parse(to_locale(language)) | ||
| break | ||
| except UnknownLocaleError: | ||
| except (UnknownLocaleError, ValueError): |
There was a problem hiding this comment.
Do we want to catch ValueError? This sounds remarkably like Except catchall but without info I'm not sure.
There was a problem hiding this comment.
It seems babel_support.Format(babel_core.Locale.parse(to_locale('en-us'))).timedelta(timedelta, format='long') is not the same as format_timedelta(timedelta, locale='en-us'). The latter actually yields a ValueError and thus the default to en. Changed PR to remove all of that.
tests/i18n/dates.py
Outdated
| locale = babel_core.Locale.parse(to_locale(language)) | ||
| break | ||
| except UnknownLocaleError: | ||
| continue |
There was a problem hiding this comment.
Does this test anything? Can we actually have a test that flags the UnkownLocaleError so we know that we're succeeding in capturing that.
There was a problem hiding this comment.
Rearranged the test to drop this code.
unho
force-pushed
the
songhay-fix-again
branch
2 times, most recently
from
55fc0f0
to
f52f980
Compare
|
lgtm |
LocalDate is dropped since the right babel locale formatter can be already retrieved by using the more reliable get_locale_formats function which provides sane defaults to fall back to in case of locale being missing in babel. The leftover code after this refactoring is simple enough to be folded into the timesince function. Also did minor refactor to get_locale_formats in order to allow telling to retrieve a specific locale, which eases testing specific locales formatting. Fixes: 'UnknownLocaleError: unknown locale 'son'
unho
force-pushed
the
songhay-fix-again
branch
from
f52f980
to
898316d
Compare
Fixes: 'UnknownLocaleError: unknown locale 'son'