Remove value parameter since icalendar skip it for default date-time

[eriol]

This PR update the format used for default date time removing VALUE=DATE-TIME since icalendar removed them as investigated by @WhyNotHugo: https://github.com/collective/icalendar/pull/479

Closes: #1205

It's currently WIP because 2 tests are failing and I need more investigation (also I would like to have CI confirming the issue). Failure seems not related to value parameter removal.

[eriol]

Removing [WIP] from title since CI reproduce the issue I was experiencing locally. I need more time to investigate.

[eriol]

Good news, I spent some time on this and the failures are related to pytz: using pytz==2022.2.1 make tests pass. Investigating further.

[WhyNotHugo]

Thanks for addressing this.

The initial set of changes LGTM. Any ideas why the last couple of tests changed their values by two months?

[eriol]

@WhyNotHugo I wrote a bit more on the commit message, I'm pasting it also here. Part of the issues were due a change in 2022g https://mm.icann.org/pipermail/tz-announce/2022-November/000076.html:

Colombia's 1993 fallback was 02-06 24:00, not 04-04 00:00.

Well, at least it's the only change related to Colombia that I saw looking at tz changelog from the version used in pytz==2022.2.1 (2022b) to the latest that is 2022g.

Do you prefer to split the PR to track pytz related issue separately?

[eriol]

@WhyNotHugo there is more to fix related to pytz unfortunately, I did not yet discover what.

[WhyNotHugo]

Thanks for the explanation.

It's fine to keep it in one PR.

I'm without computer for a few days, so can't provide much for feedback for the time being (also looks like CI logs aren't public, so can't see those).

[eriol]

Thanks for your review! No need to hurry, please take your time. I'm not pasting CI logs here since you will look at them when you will have the computer at hand!

Just to let you know what the issue seems to be: 2 tests are currently failing, in both RDATE field is missing.

I will continue to investigate during this week. Thanks again for your support.

[eriol]

I spent some time looking at free function create_timezone (inside khal/khalendar/event.py) and the problem arises here: https://github.com/pimutils/khal/blob/6012ae8060710bc500d8e286878b8797410e0131/khal/khalendar/event.py#L923-L932

In pytz==2022.2.1 tz._transition_info (tz._utc_transition_times has the same number of elements) has 6 elements and last 2 are both (datetime.timedelta(days=-1, seconds=68400), datetime.timedelta(0), '-05'):

Pdb) tz._transition_info
[(datetime.timedelta(days=-1, seconds=68640), datetime.timedelta(0), 'LMT'), (datetime.timedelta(days=-1, seconds=68640), datetime.timedelta(0), 'BMT'), (datetime.timedelta(days=-1, seconds=68400), datetime.timedelta(0), '-05'), (datetime.timedelta(days=-1, seconds=72000), datetime.timedelta(seconds=3600), '-04'), (datetime.timedelta(days=-1, seconds=68400), datetime.timedelta(0), '-05'), (datetime.timedelta(days=-1, seconds=68400), datetime.timedelta(0), '-05')]

So since we loop in range(first_num, last_num + 1) (at line 923)

and first_num and last_num are computed as https://github.com/pimutils/khal/blob/6012ae8060710bc500d8e286878b8797410e0131/khal/khalendar/event.py#L911-L920

we will loop between range(4, 6) (because first_num==4 and last_num==5) and so we will execute the loop 2 times and the last time we will able to enter inside

    if name in timezones:  # line 925

and execute the else branch that add RDATE field.

The last pytz (pytz==2022.7) instead has only 5 elements

(Pdb) tz._transition_info
[(datetime.timedelta(days=-1, seconds=68640), datetime.timedelta(0), 'LMT'), (datetime.timedelta(days=-1, seconds=68640), datetime.timedelta(0), 'BMT'), (datetime.timedelta(days=-1, seconds=68400), datetime.timedelta(0), '-05'), (datetime.timedelta(days=-1, seconds=72000), datetime.timedelta(seconds=3600), '-04'), (datetime.timedelta(days=-1, seconds=68400), datetime.timedelta(0), '-05')]

there isn't anymore the repeated element at the end, so we will loop between 4 (fist_num) and 4 (last_num) entering only once in the loop and not adding RDATE field.

Conclusion

This explains why the 2 tests are failing, but my current know how on this stuff is pretty limited, (just started peeking into khal code (after having used it for years) few days ago), so I don't know what to suggest right now.

[WhyNotHugo]

You analysis here was very useful, thanks.

Indeed, pytz.timezone('America/Bogota')._tzinfos now has only four items, and that change results in different output (in particular, the RDATE missing, which is what makes tests fail).

There's two ways to solve this:

1. Add an if to the test checking the pytz version and handling all cases (there are some existing if version.parse(pytz.__version__) > version.Version('2017.1').
2. Just leaving the value for latest pytz and let tests fail on older versions (or maybe tag these tests are "require latest pytz". I'm not sure there's any value in keeping tests passing with older pytz versions.

[WhyNotHugo]

I'll go ahead and merge these changes. https://github.com/pimutils/khal/issues/1222 exists to track the last bits.

[WhyNotHugo]

Thanks a lot for digging into these.

[eriol]

Happy to help! Thanks for khal!

[lucc]

Do you mind making a bugfix release with these changes? This would help to fix this issue downstream in NixOS.

[d7415]

Do you mind making a bugfix release with these changes? This would help to fix this issue downstream in NixOS.

1232. Probably tomorrow.