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

This adds generating VTIMEZONE components from tzinfo objects #741

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

niccokunzmann
Copy link
Member

@niccokunzmann niccokunzmann commented Nov 2, 2024

Fixes #722

  • Add icalendar.Timezone.from_tzinfo(ZoneInfo('Europe/Berlin'))
  • Add icalendar.Calendar.add_missing_timezones()
  • Identify totally unknown timezones (e.g. custom ones from ics strings)
  • make sure from_ical().to_ical() creates equivalent content

TODOs:

  • check that transition times are computed properly for pytz
    Result: This is really tricky. I do not know why pytz does not converge or why it is weird around the transition times.
  • create Calendar.add_missing_timezones
  • check with actual transition times
    Two tests check this. When we parse a calendar, at least we get the right transition times in pytz and zoneinfo.
  • issue for test_deep_copies_are_equal[issue_722_timezone_transition_ambiguity.ics-zoneinfo]
  • remove todos

@coveralls
Copy link

coveralls commented Nov 2, 2024

Pull Request Test Coverage Report for Build 11959698522

Details

  • 468 of 552 (84.78%) changed or added relevant lines in 15 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.6%) to 96.075%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/icalendar/timezone/zoneinfo.py 9 10 90.0%
src/icalendar/prop.py 10 12 83.33%
src/icalendar/tests/conftest.py 18 20 90.0%
src/icalendar/cal.py 125 128 97.66%
src/icalendar/timezone/provider.py 5 8 62.5%
src/icalendar/timezone/tzid.py 39 42 92.86%
src/icalendar/tests/test_issue_722_generate_vtimezone.py 197 205 96.1%
src/icalendar/timezone/equivalent_timezone_ids.py 24 86 27.91%
Totals Coverage Status
Change from base Build 11958391911: -1.6%
Covered Lines: 4538
Relevant Lines: 4715

💛 - Coveralls

@niccokunzmann
Copy link
Member Author

niccokunzmann commented Nov 10, 2024

I run a bit into an issue here.

I can generate them successfully for zoneinfo/dateutil.
However, when I use pytz, I run into a strange error (VTIMEZONE component != VTIMEZONE component)

pytz -> VTIMEZONE -> pytz -> VTIMEZONE

changes the transition times.

Here VTIMEZONE == VTIMEZONE

zoneinfo -> VTIMEZONE -> dateutil -> VTIMEZONE

Since these yield the same result (VTIMEZONE == VTIMEZONE):

pytz -> VTIMEZONE
zoneinfo -> VTIMEZONE

I conclude that VTIMEZONE -> pytz is actually a bit problematic. I will check this. In this case, icalendar had a bug around the transition day that the time was moved a little. e.g.

  • Europe/Berlin transitions at 3:00am/2:00am and our pytz implementation turns it into 4:00/0:00
  • Amerika/New_York transitions at 3:00am/1:00am and our pytz implementation turns it into 4:00/0:00
  • Asia/Singapore transitions at 19820101T000000 and our pytz implementation turns it into 19820101T003000 = +30min

Next steps:

  • isolate the cause of the difference
  • fix the mistake if it is ours

So, in case I cannot fix this and you depend on pytz, I still recommend that you upgrade

@niccokunzmann niccokunzmann marked this pull request as ready for review November 11, 2024 00:45
@niccokunzmann
Copy link
Member Author

I think, the interface I would like to use is this one:

Calendar().add_missing_timezones()

This will then add all the missing timezones automatically to the calendar.

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

LGTM, except for one param description.

src/icalendar/cal.py Outdated Show resolved Hide resolved
@niccokunzmann
Copy link
Member Author

The pytz parsing seems to fail when we check two calendars for equality.

   def assert_equal(actual_value, expected_value):
        """Make sure both values are equal"""
>       assert actual_value == expected_value
E       AssertionError: assert VCALENDAR({},...TE-TIME'}))})) == VCALENDAR({},...TE-TIME'}))}))
E         
E         Use -v to get more diff

src/icalendar/tests/test_equality.py:33: AssertionError
---------------------------- Captured stdout setup -----------------------------
example file: issue_722_timezone_transition_ambiguity.ics

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

A few grammar fixes, and comments.

src/icalendar/tests/test_issue_722_generate_vtimezone.py Outdated Show resolved Hide resolved
src/icalendar/tests/test_issue_722_generate_vtimezone.py Outdated Show resolved Hide resolved
src/icalendar/cal.py Outdated Show resolved Hide resolved
@niccokunzmann
Copy link
Member Author

Thank you for your reviews! I think this is ready now for the last review. The most important functionality has been added which allows us to add missing VTIMEZONE components to the calendar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate VTIMEZONE components
3 participants