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

Fix for dateutil 2.7 #798

Merged
merged 4 commits into from
Sep 18, 2018
Merged

Fix for dateutil 2.7 #798

merged 4 commits into from
Sep 18, 2018

Conversation

geier
Copy link
Member

@geier geier commented Jul 17, 2018

More information incoming

fixes #780

@Arvedui
Copy link

Arvedui commented Jul 18, 2018

This does seem to work, but you lost the comma at the end of the line.

@dvzrv
Copy link

dvzrv commented Jul 27, 2018

if this works as intended, could it be released in a new version soon?

@geier
Copy link
Member Author

geier commented Aug 2, 2018

So I finally identified the problem, but sadly haven't (implemented) a solution yet. Here is a brief description of the problem and possible solutions:

Problem description

Because of an incompatibility between dateutil's rrule and pytz's timezone we are currently removing a the timezone information before calculating the recurrence instances and applying the timezone afterwards to all the recurrence instances.

The problem occurs, when there is a localized UNTIL parameter, which is indicated by a Z (as in UTC or Zulu time) like this: FREQ=DAILY;UNTIL=20140220T060000Z'.
We hand over the complete RRULE string to datetutil's rrule function (including the UNTIL part) as well as the start date.
Since dateutil 2.7 rrule complains about the inconsistency of giving a floating (non-localized) start date, but a localized UNTIL date (in UTC).
The solution should be simple: apply the start dates timezone to the until date, then remove the timezone. BUT, until now, we didn't parse the rrule string ourselves, so we have no easy way of doing just that.

Possible solutions:

  1. monkeypatch dateutil's rrule (BAD)
  2. implement some basic parsing of RRULE strings ourselves (hard and errorprone)
  3. ship a "convenience copy" of adateutil's patched rrule :(
  4. switch to using dateutil's timezones (the icalendar library would need to switch, too)

Comments, ideas and PRs are welcome

@geier
Copy link
Member Author

geier commented Aug 2, 2018

Dateutil is BSD (and Apache 2.0) licensed

@WhyNotHugo
Copy link
Member

Why are dateutil's and pytz's timezone incompatible? Aren't they all subclasses of datetime.timezone (or rather, don't they behave like those?).

What are the changes of either upstream fixing this (eg: is there an upstream bug).

monkeypatching (option 1) sounds like the best idea to me. The only other possible solution would be to try and fix rrule upstream (or maybe both?).

@geier
Copy link
Member Author

geier commented Aug 2, 2018

@WhyNotHugo have a look at dateutil/dateutil#641

khal/utils.py Outdated
@@ -335,9 +349,12 @@ def sanitize_datetime(date):
# doesn't know any larger transition times
rrule._until = dt.datetime(2037, 12, 31)
elif getattr(rrule._until, 'tzinfo', None):
raise
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this raise ought to be here.

@geier
Copy link
Member Author

geier commented Aug 2, 2018

But good news: there is another option, read dateutil's code and discover the ignore_tz option. This needs some more working around some issues, but this is mostly fixed.

Now this showed that we have a bug (i.e., we generate non compliant .ics files) in the new --until code, which I don't completely understand yet (the code, not the bug).

@Holzhaus
Copy link

Holzhaus commented Aug 9, 2018

IMHO, ignoring the timezone is a valid short term workaround. However, the proper fix (long term) should be to move away from pytz and switch to dateutil's timezones.

@geier geier merged commit 7932721 into master Sep 18, 2018
geier added a commit that referenced this pull request Sep 19, 2018
asergi added a commit to void-linux/void-packages that referenced this pull request Sep 29, 2018
@legrostdg
Copy link

Would it be possible to release a new version of khal with this fix? It would help in keeping khal in Debian (it will be removed from testing today if there is no fix to https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=908497)

@geier
Copy link
Member Author

geier commented Oct 12, 2018

@legrostdg sorry, forgot to notify you, I actually did a release on that day, which is only available on pypi for the moment.

@legrostdg
Copy link

great, thanks!

@geier geier deleted the dateutil2.7 branch March 25, 2019 20:49
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.

Compatibiltiy with dateutil 2.7
6 participants