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

Fix dates formatting Y, w and W symbols for week-numbering #1179

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

jun66j5
Copy link
Contributor

@jun66j5 jun66j5 commented Jan 29, 2025

Fix #1133.

I confirmed the added test cases for week-numbering are correct using PyICU library.

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.50%. Comparing base (2d8a808) to head (3175839).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1179      +/-   ##
==========================================
+ Coverage   91.37%   91.50%   +0.13%     
==========================================
  Files          27       27              
  Lines        4672     4687      +15     
==========================================
+ Hits         4269     4289      +20     
+ Misses        403      398       -5     
Flag Coverage Δ
macos-14-3.10 90.54% <100.00%> (+0.15%) ⬆️
macos-14-3.11 90.48% <100.00%> (+0.15%) ⬆️
macos-14-3.12 90.69% <100.00%> (+0.15%) ⬆️
macos-14-3.13 90.69% <100.00%> (+0.15%) ⬆️
macos-14-3.8 90.41% <100.00%> (+0.15%) ⬆️
macos-14-3.9 90.47% <100.00%> (+0.15%) ⬆️
macos-14-pypy3.10 90.54% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-3.10 90.56% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-3.11 90.50% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-3.12 90.71% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-3.13 90.71% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-3.8 90.43% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-3.9 90.49% <100.00%> (+0.15%) ⬆️
ubuntu-24.04-pypy3.10 90.56% <100.00%> (+0.15%) ⬆️
windows-2022-3.10 90.55% <100.00%> (+0.13%) ⬆️
windows-2022-3.11 90.49% <100.00%> (+0.13%) ⬆️
windows-2022-3.12 90.70% <100.00%> (+0.13%) ⬆️
windows-2022-3.13 90.70% <100.00%> (+0.13%) ⬆️
windows-2022-3.8 90.53% <100.00%> (+0.13%) ⬆️
windows-2022-3.9 90.48% <100.00%> (+0.13%) ⬆️
windows-2022-pypy3.10 90.55% <100.00%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I confirmed the added test cases for week-numbering are correct using PyICU library.

Could you share the code you used for that?

Some lines that you added are not covered by the tests, could you test them as well?

babel/dates.py Outdated Show resolved Hide resolved
babel/dates.py Outdated Show resolved Hide resolved
babel/dates.py Outdated
@@ -1677,6 +1675,33 @@ def get_day_of_year(self, date: datetime.date | None = None) -> int:
date = self.value
return (date - date.replace(month=1, day=1)).days + 1

def get_week_of_year(self, value: datetime.datetime | datetime.date | None = None) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

Should these methods be public or do we want to keep them private? Either way, it might be nice to add a docstring and some examples perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these methods should be kept private.

If there is a use case that determines week of year/month from a date/datetime instance, we should add them as helper methods to babel/dates.py, not instance methods of DateTimeFormat.

@jun66j5
Copy link
Contributor Author

jun66j5 commented Jan 30, 2025

I confirmed the added test cases for week-numbering are correct using PyICU library.

Could you share the code you used for that?

import datetime
import icu

def icu_format(value, format, locale):
    if isinstance(locale, str):
        locale = icu.Locale(locale)
    tz = icu.TimeZone.getGMT()
    cal = icu.Calendar.createInstance(tz, locale)
    f = icu.SimpleDateFormat(format, locale)
    if isinstance(value, datetime.date):
        cal.set(value.year, value.month - 1, value.day)
    elif isinstance(value, datetime.datetime):
        cal.set(value.year, value.month - 1, value.day, value.hour, value.minute, value.second)
    else:
        raise ValueError(repr(value))
    return f.format(cal.getTime())
>>> icu_format(datetime.date(2023, 12, 31), "YYYY-'W'ww-e", 'de_DE')
'2023-W52-7'
>>> icu_format(datetime.date(2023, 12, 31), "YYYY-'W'ww-e", 'en_US')
'2024-W01-1'
>>> icu_format(datetime.date(2023, 12, 31), "YYYY-'W'ww-e", 'en_AU')
'2023-W53-7'

Some lines that you added are not covered by the tests, could you test them as well?

I just moved to the get_week_of_month method from the formatting logic for W symbol in format_week. Also, Babel has no tests for the W....

Trying to add tests for the week of month, however results of ICU and Babel differ. I think one or both are incorrect. I'm unsure why.

>>> icu_format(datetime.date(2003, 1, 31), 'W', 'de_DE')
'5'
>>> icu_format(datetime.date(2003, 2, 1), 'W', 'de_DE')
'0'   # <= I think it should be '1'
>>> format_date(parse_date('2003-01-31'), format='W', locale='de_DE')
'5'
>>> format_date(parse_date('2003-02-01'), format='W', locale='de_DE')
'5'   # <= I think it should be '1'

@tomasr8
Copy link
Member

tomasr8 commented Jan 30, 2025

>>> format_date(parse_date('2003-02-01'), format='W', locale='de_DE')
'5'   # <= I think it should be '1'

Maybe W is getting confused with w somehow? (week of year)

@jun66j5 jun66j5 force-pushed the fix-format-week-dates branch from 7ccfdf7 to d5f31af Compare January 30, 2025 08:36
@jun66j5
Copy link
Contributor Author

jun66j5 commented Jan 30, 2025

>>> format_date(parse_date('2003-02-01'), format='W', locale='de_DE')
'5'   # <= I think it should be '1'

Maybe W is getting confused with w somehow? (week of year)

The lack of the coverage is for get_week_of_month.

Symbol Description Method
W week of month get_week_of_month
w week of year get_week_of_year

@jun66j5
Copy link
Contributor Author

jun66j5 commented Jan 30, 2025

Trying to add tests for the week of month, however results of ICU and Babel differ. I think one or both are incorrect. I'm unsure why.

>>> icu_format(datetime.date(2003, 1, 31), 'W', 'de_DE')
'5'
>>> icu_format(datetime.date(2003, 2, 1), 'W', 'de_DE')
'0'   # <= I think it should be '1'
>>> format_date(parse_date('2003-01-31'), format='W', locale='de_DE')
'5'
>>> format_date(parse_date('2003-02-01'), format='W', locale='de_DE')
'5'   # <= I think it should be '1'

Reconsidering with week-of-month calculation in Week Data section in TR#35, it seems that ICU is correct. I fixed week-of-month calculation in Babel in the additional commit of this pullreq.

@jun66j5 jun66j5 changed the title Fix datetime formatting Y and w symbols for week-numbering Fix dates formatting Y, w and W symbols for week-numbering Jan 30, 2025
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

I also cross-checked these results with ICU4C 76, and they, well, check out. ✅

Thank you for your hard work on this, @jun66j5!

@akx akx merged commit 363ad75 into python-babel:master Jan 31, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issue with en_AU locale
3 participants