Fix timezone identification for dateutil timezones with incorrect his… #1021
Conversation
…torical definitions - Add fallback mechanism that identifies timezones by utcoffset behavior - Uses midday (12:00) instead of midnight to avoid ambiguous transitions - Fixes issues collective#763, collective#775, collective#776, collective#780, collective#791 - Add comprehensive test suite with 11 test cases - Update changelog and credits
mikenj6
requested review from
SashankBhamidi,
abe-101,
angatha,
niccokunzmann and
stevepiercy
as code owners
| - Ville Skyttä <[email protected]> | ||
| - Wichert Akkerman <[email protected]> | ||
| - Howard Liao <[email protected]> | ||
| - Michael Njoku <[email protected]>`_ |
There was a problem hiding this comment.
Please use the GitHub URL for your username so that you don't disclose your email address to spam harvesters. You're welcome.
Also please sort your name alphabetically in the list.
| - Fix timezone identification for dateutil timezones with incorrect historical definitions or missing filename attributes | ||
| - Added fallback mechanism that identifies timezones by their utcoffset behavior at different historical points. | ||
| - Uses midday (12:00) instead of midnight to avoid ambiguous timezone transitions, building on the approach from issue #776. | ||
| - See `Issue 763 <https://github.com/collective/icalendar/issues/763>`_, `Issue 775 <https://github.com/collective/icalendar/issues/775>`_, `Issue 776 <https://github.com/collective/icalendar/issues/776>`_, `Issue 780 <https://github.com/collective/icalendar/issues/780>`_, and `Issue 791 <https://github.com/collective/icalendar/issues/791>`_. |
There was a problem hiding this comment.
Reference only one issue per line. It's difficult to understand which narrative text aligns with its issue number in this format.
|
Nice. I had a read... This actually uses the lookup tree with some more flexibility. So, technically, this is sound. I just wonder when the tests will be running... this is a bit weird. They all seem to wait. #780 will not be fixed by this. @mikenj6 Why did you decide to use the lookup tree although it is likely to be outdated with a new release of timezone definitions? Did you use TDD to fix this? Thanks! |
niccokunzmann
added
the
ai-suspicion
@niccokunzmann I recently changed https://github.com/collective/icalendar/settings/branch_protection_rules/51525110. @SashankBhamidi and I were enabling auto-merge on pull requests, and we found that when it's enabled and even when "Require status checks to pass before merging" is enabled, it would still auto-merge before the checks had completed and passed. This had the potential to introduce untested bugs. The fix was to explicitly choose which status checks must pass before branches can be merged. The selected checks are exactly those which have not yet run. I think that change also applies to "Require branches to be up to date before merging" when it is enabled. That makes sense. It won't bother to run the test suite on outdated code, which would be a waste of CPU and energy. |
| from datetime import datetime, tzinfo | ||
|
|
||
| DATEUTIL_UTC = tz.gettz("UTC") | ||
| DATEUTIL_UTC_PATH: Optional[str] = getattr(DATEUTIL_UTC, "_filename", None) |
There was a problem hiding this comment.
This syntax was introduced with python 3.10 but icalendar targests 3.8
4 participants
Closes issues #763, #775, #776, #780, #791
Description
I fixed timezone identification for dateutil timezones that have incorrect historical definitions or missing filename attributes. The fix adds a fallback mechanism in
tzids_from_tzinfo()that identifies timezones by their utcoffset behavior at different historical points, even when they have incorrect historical data.Changes made:
_identify_tzid_by_behavior()function insrc/icalendar/timezone/tzid.pythat traverses the lookup tree based on utcoffset behaviortzids_from_tzinfo()when filename-based identification failstest_issue_763_timezone_identification_historical.pywith 11 test casesCHANGES.rstwith bug fix entrydocs/contribute/credits.rstProblem solved:
dateutil timezones (like EET) can return incorrect offsets for historical dates (e.g., 1983-03-27 returns 10800 seconds instead of 7200), making it impossible to identify them by behavior alone. This was causing test failures and instability.
Checklist
Additional information
Testing:
Related Issues:
Asia/Manilatests fail with the latest release #791: Asia/Manila tests fail with the latest release📚 Documentation preview 📚: https://icalendar--1021.org.readthedocs.build/