Datetimes with timezones from dateutil.tz badly supported

[tobixen]

See also #333.

Here is some test code to illustrate the issue:

import datetime
import pytz
from dateutil import tz
import icalendar

utc1 = pytz.utc
utc2 = tz.UTC
oslo1 = pytz.timezone('Europe/Oslo')
oslo2 = tz.gettz('Europe/Oslo')
remote1 = pytz.timezone('Brazil/DeNoronha')
remote2 = tz.gettz('Brazil/DeNoronha')

for tz in (utc1, utc2, oslo1, oslo2, remote1, remote2):
    myevent = icalendar.Event()
    myevent.add('dtstart', datetime.datetime.now().astimezone(tz))
    print(myevent.to_ical().decode('ASCII'))

Particularly the 'remote2' output line is quite unacceptable ...

DTSTART;TZID=-02;VALUE=DATE-TIME:20211117T133359

There seems to be no easy fix for this, since there seems to be no obvious way to get from tz.gettz('Brazil/DeNoronha') to Brazil/DeNoronha. (It's not just me being eurocentric - DeNoronha is really one of the most remote time zones for most of the world population, just check the map). I see that the tzinfo object from dateutil has a file name - perhaps it's possible to reverse-engineer something from the filename - but it will be hard to make a platform-independent solution.

I could perhaps try to work out something, but only if someone (preferably someone with the authority to merge pull requests) says it's a good idea.

[pjkaufman]

@tobixen , is this similar to #291 ? It sounds to me like you want the timezone name from a ZoneInfo. I think I suggested a potential solution on the issue I mentioned, but I have not tried it out on this repo yet. I have tested it on python 3.9 though and it seems to get the desired result. Please let me know if there is a difference between these issues. Thanks!

[tobixen]

Your potential solution is something of the same as I've done in pull request #334 and it will work fine for "new style" timezone objects coming from the new zoneinfo library. However, it will still break with timezone objects created through the old dateutil library.

[tobixen]

Almost but not completely the same. I'm using the key property of the timezone object, @pjkaufman is casting to str.

Checking the zoneinfo code, I find this:

    def __str__(self):
        if self._key is not None:
            return f"{self._key}"
        else:
            return repr(self)

So it is possible to create a zoneinfo object from a file using the from_file class method, such an object won't have a key and in that case it seems more or less impossible to create the timezone label ... just like with the dateutil.tz-objects referred to above. Perhaps the best workaround for such corner cases would be to transform the datetime itself into UTC.

Falling back to repr does not quite solve the problem, __repr__ will create some gibberish that cannot be used in the ical object. Hence, neither "my" usage of tz.key or "your" str(tz) seems to solve this potentially problem fully.

repr will return f"{self.__class__.__name__}.from_file({self._file_repr})" if no key is set.

And no, str is no solution for dateutil.tz-objects either:

>>> from dateutil.tz import gettz
>>> str(gettz('Europe/Helsinki'))
"tzfile('/usr/share/zoneinfo/Europe/Helsinki')"

It could be possible to assume the last parts of the file name is identical to the zone name, but one cannot rely on that.

[pjkaufman]

@tobixen , that is very interesting. I am a little new to any kind of python development, so I may not be of much help as a whole on figuring out some of the nuances. However, I am more than willing to help out anywhere I can.

I seem to have problems getting the UTs to pass locally for this repo. Is there something stopping the progress on #334? I see some of the CI checks are failing.

[tobixen]

Unit tests were passing locally for me, I never got smart on the CI breakages in #334, but it seems to me they are arbitrary (different tests breaking on each run) and not at all related to the icalendar project. shrug ...

I'm sort of just waiting for someone in the icalendar/plone project to pick up on it and comment, in the meanwhile the workaround is to use the deprecated pytz library for generating time zones in my projects.

[pjkaufman]

Have you been able to run it on the versions it was braking on? I know that it could be that those versions are failing. Though, I am a little new to python UTs, so maybe it automatically runs it on each version.

[tobixen]

The tests that are broken are testing the plone framework, the unit tests for the icalendar library itself seems to be passing. I'm not much motivated for looking into the plone testing framework currently.

[niccokunzmann]

I add support for zoneinfo in #623. There is a way to switch between zoneinfo and dateutil.tz. In fact, I use dateutil.tz.tzical to parse the custom timezones that are not known by zoneinfo. Thus, it would probably be good to also test dateutil.tz and make sure we can use the whole codebase with that one.

Also, now we do not run the plone tests any more so if you want to give it another go.

What are your thoughts on this?

[tobixen]

I think that the original bug I reported is actually a bug in the tzutil library ...

>>> remote2 = tz.gettz('Brazil/DeNoronha')
>>> remote2
tzfile('/usr/share/zoneinfo/Brazil/DeNoronha')
>>> remote2.tzname(datetime.datetime.now())
'-02'

It does have a method tzname, but it does not give any sensible information. The name of the time zone is given when I call tz.gettz , but the library throws it away. It is also possible to deduct from the file name.

This may be a side track - actually, the icalendar RFC does not mandate neither clients nor servers to know anything about the Olson database, proper icalendar should actually include the timezone specification in the VCALENDAR.

[tobixen]

But yes, supporting modern zoneinfo-objects is a good idea. Pytz is deprecated, and dateutil is outside the standard library and gives weird results as I've shown above. Personally I don't really mind that this change may break backward compatibility in some scenarios.

[niccokunzmann]

@tobixen, I used dateutil for #623 to parse the ICS timezone input into a tzinfo object. I wonder: Could you take a look at the code and see if we will face any issues from that:

https://github.com/collective/icalendar/pull/623/files#diff-a162a419c284a20838353f733f3f53fe9fbb077196acf7c078c332ac294e6a7dR72

Also, it might be worth reporting this on the dateutil side so they know about it and a fix can be implemented in the right places.

What are your thoughts on this?

[tobixen]

Right. That's a big diff. I'll try to wrap my head around it.

I didn't consider reporting the issue upstream as I thought dateutil.tz.gettz('Europe/Sofia) was being deprecated with zoneinfo.ZoneInfo('Europe/Sofia') as the better way to do things forward. But yes, it's probably an idea.

[niccokunzmann]

icalendar/src/icalendar/prop.py

Line 87 in 0cc0e1b

def tzid_from_dt(dt: datetime) -> Optional[str]:

might be the place to start.

We might need to add to dateutil in order to achieve what we want as well as adding patch code to make it nicely accessible.