-
-
Notifications
You must be signed in to change notification settings - Fork 405
Conversation
Add a PYICU implementation of TZProvider backed by PyICU/ICU, plus use_pyicu() wiring in TZP and the icalendar.timezone package. Working so far: timezone lookup via icu.ICUtzinfo, UTC handling, knows_timezone_id, unknown id -> None, and create_timezone via icu.VTimeZone.createVTimeZone. A VTIMEZONE that ICU cannot parse raises a ValueError instead of crashing the interpreter. Still TODO (follow-up commits): parametrize conftest.py over the pyicu backend, add PyICU-specific tests, declare the optional test dependency, and lint/type-hint cleanup. AI use disclosure: I used Claude Opus 4.8 (via Claude Code) to explain the icalendar timezone provider architecture and to verify the provider's behaviour by running it. I wrote the code myself. Refs collective#692
This pull request did not pass quality checks and AI use is suspected. Please review Contribute and make any necessary amendments.
Profile summary:
GitHub user: h4syy
🟢 No concerns found with user's profile.
🟢 No concerns found with recent PR activity.
🟢 No concerns found with recent issue activity.
For a more detailed report, run `gh-profiler h4syy`.
Full profile
GitHub user: h4syy
🟢 No concerns found with user's profile.
🟢 Account age: 5 years
🟢 Profile information:
name: Yash Paudel
company: Khalti
linkedin: https://www.linkedin.com/in/yashpaudel/
Empty fields: blog, location, email, bio
🟢 No concerns found with recent PR activity.
2 PRs opened in the last 21 days.
0 opened against repos the user owns.
0 opened against repos in publicly associated orgs.
2 opened against external repos.
🟢 1 of 2 external PRs merged in the last 21 days.
🟢 0 of 2 external PRs closed without merging in the last 21 days.
🟢 No concerns found with recent issue activity.
6 new issues opened in the last 21 days.
6 opened in repos the user owns.
0 opened in repos in publicly associated orgs.
0 opened in external repos.
🟢 0 external issues closed as NOT_PLANNED.
🟢 0 external issues opened with the same title.
Hi, thanks for the PR!
Some answers:
uses_pyicu() - yes, they all should have the same interface.
issue_321_assert_dst_offset_is_not_false.ics - if a fix cannot be done because it is too much for this PR, then skipping is a good way to work this this (and preferably adding a TODO in the code or an issue if it is important). Best would be to somehow fix this - either in icalendar or in PyICU itself - report this upstream. Generally all files should work with all timezones. The reason is that we collect the different quirks and wild (mis-)interpretations of the RFC standard and want to take the burden off developers to deal with these. So, that special case is best handeled in a consistent way with the others by calendar, so there is no deviation. If you think, this should be handled by PyICU instead of icalendar, create an issue in PyICU to fix it and add a skip with a reference to that issue so it can be removed when the issue is closed.
If it is hard to install, then, I would say the tests should check - maybe like with pytz - wether pyicu is installed. It is also a question of documentation but some kind of switch is required, so they can be switched on. Is it hard to install PyICU on linux? What is the easiest way to install it?
Let me know when you would like me to review this.
Linked issue
Description
Adds a PyICU timezone provider — a
PYICUimplementation ofTZProvider, selectable viause_pyicu()and wired throughTZPand theicalendar.timezonepackage. This is part 1 of #692 and is opened as a draft / work in progress.Working so far (verified locally against the repo's VTIMEZONE fixtures — 37/38
create_timezonesucceed):icu.ICUtzinfo, UTC handling, andknows_timezone_idNone, so the Windows→Olson and globally-unique-TZID fallbacks inTZPstill workcreate_timezoneparses theVTIMEZONEviaicu.VTimeZone.createVTimeZone(DST transitions correct); aVTIMEZONEthat ICU cannot parse raisesValueErrorinstead of segfaulting the interpreterStill to do (follow-up commits): parametrize
conftest.pyacross the pyicu backend, add PyICU-specific tests, declare the optional PyICU test dependency (gated so it skips when ICU is absent), resolveuses_pyicu(), and lint/type-hint cleanup.Questions I'd love input on, @niccokunzmann (I'll also add inline comments on the lines):
create_timezoneusesicu.VTimeZone.createVTimeZoneas you suggested. One fixture —issue_321_assert_dst_offset_is_not_false.ics(Europe/Berlin) — is rejected by ICU (empty id) where zoneinfo/pytz succeed. Prefer a fallback for those, or anxfailfor pyicu?uses_pyicu(): add it to theTZProviderABC +TZPproxy for symmetry withuses_pytz/uses_zoneinfo, or keep providers reporting only pytz/zoneinfo?Checklist
Additional information
Draft / WIP — tests, docs, and the conftest parametrization are coming in follow-up commits.
AI disclosure: I used Claude (Opus 4.8, via Claude Code) to explain the timezone-provider architecture and to verify the provider's behaviour by running it; the code is my own. This is also noted in the commit message and the
news/692.featureentry.