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.

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

KeyError raised from format_time() with zh_TW on Babel 2.3.1 #378

Closed
jun66j5 opened this issue Apr 7, 2016 · 4 comments
Closed

KeyError raised from format_time() with zh_TW on Babel 2.3.1 #378

jun66j5 opened this issue Apr 7, 2016 · 4 comments
Labels
bug

Comments

@jun66j5
Copy link
Contributor

@jun66j5 jun66j5 commented Apr 7, 2016

(Originally, reported in https://trac.edgewall.org/ticket/12445)
This issue doesn't occur on Babel 2.2.0.

>>> import babel
>>> babel.__version__
'2.3.1'
>>> from datetime import datetime
>>> from babel.dates import format_time
>>> format_time(datetime(2016, 4, 8, 12, 34, 56), locale='zh_TW')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/dev/shm/babel231/local/lib/python2.7/site-packages/babel/dates.py", line 787, in format_time
    return parse_pattern(format).apply(time, locale)
  File "/dev/shm/babel231/local/lib/python2.7/site-packages/babel/dates.py", line 1208, in apply
    return self % DateTimeFormat(datetime, locale)
  File "/dev/shm/babel231/local/lib/python2.7/site-packages/babel/dates.py", line 1205, in __mod__
    return self.format % other
  File "/dev/shm/babel231/local/lib/python2.7/site-packages/babel/dates.py", line 1242, in __getitem__
    return self.format_period(char)
  File "/dev/shm/babel231/local/lib/python2.7/site-packages/babel/dates.py", line 1384, in format_period
    return get_period_names(locale=self.locale)[period]
  File "/dev/shm/babel231/local/lib/python2.7/site-packages/babel/localedata.py", line 207, in __getitem__
    orig = val = self._data[key]
KeyError: 'pm'

I guess that format_period() should pass context='format' to get_period_names().

diff --git a/babel/dates.py b/babel/dates.py
index 3c75b38..64e7b01 100644
--- a/babel/dates.py
+++ b/babel/dates.py
@@ -1381,7 +1381,7 @@ class DateTimeFormat(object):

     def format_period(self, char):
         period = {0: 'am', 1: 'pm'}[int(self.value.hour >= 12)]
-        return get_period_names(locale=self.locale)[period]
+        return get_period_names(context='format', locale=self.locale)[period]

     def format_frac_seconds(self, num):
         """ Return fractional seconds.
@jtwang jtwang added the bug label Apr 8, 2016
@jtwang
Copy link
Contributor

@jtwang jtwang commented Apr 8, 2016

Thanks for the awesome details! It really helped a lot. :)

Anyhow, this was probably introduced with the day period API change.

Prior to this change, it looks like Locale.periods would fold all dayPeriodContexts together that had dayPeriodWidth. Now we separate the contexts.

So, for zh_TW, if we look at zh_Hant.xml's gregorian calendar settings, we can see why it broke:

  • format + wide has values defined for am and pm
  • stand-alone + wide does not have values defined for am and pm

However, I think the underlying problem is that the day period settings are not falling back to zh.xml, which does have am defined for both contexts.

>>> babel.Locale.parse('zh_TW').day_periods['stand-alone']['wide'].keys()
['morning1', 'morning2', 'night1', 'midnight', 'afternoon2', 'afternoon1', 'evening1']

>>> babel.Locale.parse('zh').day_periods['stand-alone']['wide'].keys()
['morning1', 'morning2', 'night1', 'midnight', 'am', 'afternoon1', 'afternoon2', 'evening1', 'pm']

I'm also still missing a piece of the puzzle -> dayPeriods.xml does not require am/pm for zh, so I'm not entirely sure why it's defined in zh.xml in the first place.

I can pick this back up tomorrow, although I will not complain if someone else does before then. :)

akx added a commit to akx/babel that referenced this issue Apr 8, 2016
akx added a commit to akx/babel that referenced this issue Apr 8, 2016
akx added a commit to akx/babel that referenced this issue Apr 8, 2016
`zh_Hant` locale data does not have names for the
`am` and `pm` day periods in the `format`/`abbreviated` context,
so fallback logic is added to deal with that eventuality.

Fixes python-babel#378
akx added a commit to akx/babel that referenced this issue Apr 8, 2016
@akx
Copy link
Member

@akx akx commented Apr 8, 2016

@jun66j5: Thanks for the bug report and sorry for the inconvenience 😞 However I think I have a fix for this in #379 😁

Just to verify though: 下午12:34:56 ('\u4e0b\u534812:34:56') is the expected formatting? (That's what Babel output before the day period overhaul.)


@jtwang: I think one piece of the puzzle that you're missing with your sleuthing there (and thanks for the pointers btw! :) ) is that there's actually a parent chain exception for zh_Hant; it does not derive from zh at all, but from root:

<!-- ✂️ -->
<parentLocale parent="root" locales="az_Arab az_Cyrl bm_Nkoo bs_Cyrl en_Dsrt en_Shaw ha_Arab iu_Latn mn_Mong ms_Arab pa_Arab shi_Latn sr_Latn uz_Arab uz_Cyrl vai_Latn zh_Hant"/>
<!-- ✂️ -->

However I still think the zh_Hant locale data is slightly buggy -- see the conspicuous lack of am and pm in abbreviated:

<!-- ✂️ -->
<dayPeriodContext type="format">
    <dayPeriodWidth type="abbreviated">
        <dayPeriod type="midnight">午夜</dayPeriod>
        <dayPeriod type="morning1">清晨</dayPeriod>
        <dayPeriod type="morning2">上午</dayPeriod>
        <dayPeriod type="afternoon1">中午</dayPeriod>
        <dayPeriod type="afternoon2">下午</dayPeriod>
        <dayPeriod type="evening1">晚上</dayPeriod>
        <dayPeriod type="night1">凌晨</dayPeriod>
    </dayPeriodWidth>
    <dayPeriodWidth type="narrow">
        <dayPeriod type="midnight">午夜</dayPeriod>
        <dayPeriod type="am">上午</dayPeriod>
        <dayPeriod type="pm">下午</dayPeriod>
        <dayPeriod type="morning1">清晨</dayPeriod>
        <dayPeriod type="morning2">上午</dayPeriod>
        <dayPeriod type="afternoon1">中午</dayPeriod>
        <dayPeriod type="afternoon2">下午</dayPeriod>
        <dayPeriod type="evening1">晚上</dayPeriod>
        <dayPeriod type="night1">凌晨</dayPeriod>
    </dayPeriodWidth>
    <dayPeriodWidth type="wide">
        <dayPeriod type="midnight">午夜</dayPeriod>
        <dayPeriod type="am">上午</dayPeriod>
        <dayPeriod type="pm">下午</dayPeriod>
        <dayPeriod type="morning1">清晨</dayPeriod>
        <dayPeriod type="morning2">上午</dayPeriod>
        <dayPeriod type="afternoon1">中午</dayPeriod>
        <dayPeriod type="afternoon2">下午</dayPeriod>
        <dayPeriod type="evening1">晚上</dayPeriod>
        <dayPeriod type="night1">凌晨</dayPeriod>
    </dayPeriodWidth>
</dayPeriodContext>
<!-- ✂️ -->
akx added a commit to akx/babel that referenced this issue Apr 8, 2016
@jun66j5
Copy link
Contributor Author

@jun66j5 jun66j5 commented Apr 8, 2016

Just to verify though: 下午12:34:56 ('\u4e0b\u534812:34:56') is the expected formatting? (That's what Babel output before the day period overhaul.)

Okay to me.

However, I think it would be good to check with order of wide, abbreviated, narrow. (narrow is shortest)

>>> for w in ('wide', 'narrow', 'abbreviated'):
...  print((w, get_period_names(context='format', width=w, locale='en_US')['am']))
...
('wide', u'AM')
('narrow', u'a')
('abbreviated', u'AM')

However I still think the zh_Hant locale data is slightly buggy -- see the conspicuous lack of am and pm in abbreviated:

According to http://unicode.org/cldr/trac/ticket/9017, that issue is fixed in CLDR Release 29.

@akx akx closed this in c028a09 Apr 8, 2016
akx added a commit that referenced this issue Apr 8, 2016
Fix day period formatting (#378)
@akx
Copy link
Member

@akx akx commented Apr 8, 2016

@jun66j5 That happens to be what the patch does in any case, so yay. :)

Please hold for 2.3.2, to be released Very Soon Now :)

sils added a commit that referenced this issue Apr 8, 2016
`zh_Hant` locale data does not have names for the
`am` and `pm` day periods in the `format`/`abbreviated` context,
so fallback logic is added to deal with that eventuality.

Fixes #378
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants