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

TZID emissions don't match timezone names, but may later result in a timezone name lookup #291

Closed
Lx opened this issue Jun 10, 2019 · 9 comments · Fixed by #339
Closed

Comments

@Lx
Copy link

Lx commented Jun 10, 2019

When creating an event with a timezone-aware datetime object in the Australia/Melbourne timezone, the resultant line in the iCalendar file looks like this:

DTSTART;TZID=AEST;VALUE=DATE-TIME:20190611T103000

Note that AEST is emitted in the output rather than Australia/Melbourne.

The value used in TZID would normally be completely irrelevant, because it's normally only intended to identify a corresponding VTIMEZONE component elsewhere in the calendar. However, in Issue #44, it seems to be stated that the icalendar library will try to resolve the TZID value as a pytz timezone before looking at the included VTIMEZONE components.

In light of this, the fact that TZID=AEST is emitted above instead of TZID=Australia/Melbourne feels to me almost certainly in error:

  1. The abbreviated value ("AEST" in this case) is ambiguous and fails a pytz lookup:

    pytz.timezone('AEST')
    Traceback (most recent call last):
      File "venv/lib/python3.7/site-packages/pytz/__init__.py", line 181, in timezone
        raise UnknownTimeZoneError(zone)
    pytz.exceptions.UnknownTimeZoneError: 'AEST'
    
  2. iCloud, for example, uses the whole zone name:

    DTSTART;TZID=Australia/Melbourne:20190521T170000
    

Is this an icalendar library bug?

@pjkaufman
Copy link

This is an issue I am running into with an application that uses this as a dependency. This seems to be a bug to me.

@pjkaufman
Copy link

pjkaufman commented Nov 3, 2021

I think the solution would be to do str(dt.tzinfo) instead of dt.tzinfo.tzname(dt) here: https://github.com/collective/icalendar/blob/master/src/icalendar/parser.py#L58

@mauritsvanrees @jdufresne, what do you think about the proposed change? I know it works for version 3.9 of python, but I am not sure about previous versions.

@mauritsvanrees
Copy link
Member

Sorry, I just occasionally create a release, I don't really understand how this package works or what the correct output would be in this case.

@pjkaufman
Copy link

@mauritsvanrees , if I were to make the change myself, would you be able to approve the PR or know of someone that could approve the PR to get this issue fixed?

@mauritsvanrees
Copy link
Member

I can approve a PR if I feel comfortable enough about it.

It would help if we had a better test setup with GitHub Actions for all Python versions we support, instead of relying on the Jenkins server, which runs the full Plone test suite on the Python versions that Plone supports. I have trouble getting GHA to pass though in this PR I made for it: #340

@pjkaufman
Copy link

pjkaufman commented Dec 21, 2021

@mauritsvanrees , sorry about the delay in responding. I did not see the PR you had made. I can try to test out a change locally, but I will need to be able to run the test suite locally.

Could you explain why you are removing support for certain versions of python in #340 ?

@tobixen
Copy link
Contributor

tobixen commented Dec 21, 2021

Oh, I may have raised some duplicated issues here. I noticed the same issue and did some research into it in #333, #334, #335 and #336.

@tobixen
Copy link
Contributor

tobixen commented Dec 21, 2021

Timezones have become quite a mess in python, we have three different providers of timezone data (pytz, zoneinfo, dateutil.zoneinfo) and they all behave different. Currently the icalendar library only supports pytz (which is deprecated). With my pull request in #334, the "modern" zoneinfo objects will be supported. dateutil.zoneinfo seems harder to support (#336).

@tobixen
Copy link
Contributor

tobixen commented Dec 21, 2021

I think the solution would be to do str(dt.tzinfo) instead of dt.tzinfo.tzname(dt) here: https://github.com/collective/icalendar/blob/master/src/icalendar/parser.py#L58

This is basically what I did in pull request #334.

@jacadzaca jacadzaca linked a pull request Sep 5, 2022 that will close this issue
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 a pull request may close this issue.

5 participants