Refactor MatchTimeZone method in RecurrencePatternEvaluator
#664
Conversation
* Simplified handling of UNTIL value in ProcessRecurrencePattern. * Refactored MatchTimeZone to ensure UNTIL matches DTSTART's time zone and type. * Added unit tests to verify behavior under various scenarios, improving accuracy and consistency of recurrence evaluations. Closes ical-org#663
Ical.Net.Tests/MatchTimeZoneTests.cs
Outdated
| BEGIN:VEVENT | ||
| UID:example1 | ||
| SUMMARY:Event with local time and time zone | ||
| DTSTART;TZID=Asia/Tokyo:20231101T090000 |
There was a problem hiding this comment.
In the test above with NY tz you chose dates involving a DST change (in 2023 in NY winter DST change was Nov 5), but in this test no DST change is involved (there is no DST in Tokyo). Not a problem, just noting this to make sure, its intentional.
There was a problem hiding this comment.
Good point. I updated the test to use Sydney/Australia with DST change.
| October 4, 2024: 01:00 AM - 02:00 AM (UTC+1000) (Australia/Sydney) | ||
| October 5, 2024: 01:00 AM - 02:00 AM (UTC+1000) (Australia/Sydney) | ||
| October 6, 2024: 01:00 AM - 02:00 AM (UTC+1100) (Australia/Sydney) |
There was a problem hiding this comment.
Oh, actually I think, this should be included, because UNTIL must be considered inclusive.
If the value
specified by UNTIL is synchronized with the specified recurrence,
this DATE or DATE-TIME becomes the last instance of the
recurrence.
There was a problem hiding this comment.
Yes, the conclusion came from the description I assume. It was not updated.
Now there are 2 test cases, one when DST occurs.
There was a problem hiding this comment.
Yes, I was looking at the comments. But now reconsidering the test, I still think its not correct.
October 6, 2024: 01:00 AM Sydney time = 20241005T140000Z, so it should be included in the first test and the number of occurrences should therefore be 6 in both cases.
There was a problem hiding this comment.
Isn't UTC Time: 2024-10-05 14:00:00 => Sydney Time (AEST, UTC+10:00): 2024-10-06 00:00:00?
There was a problem hiding this comment.
Oh yes, of course, sorry for the confusion!
axunonb
force-pushed
the
pr/fix-match-timezone
branch
from
4b529bf to
72bb513
Compare
|
| System.Globalization.CultureInfo.InvariantCulture, | ||
| System.Globalization.DateTimeStyles.AssumeUniversal | | ||
| System.Globalization.DateTimeStyles.AdjustToUniversal); | ||
| var occurrences = evt.GetOccurrences(new CalDateTime(2024, 10, 01), new CalDateTime(2024, 10, 07)); |
There was a problem hiding this comment.
In this case DTSTART has a tzid but the startTime is floating. This is quite a special case and we should considere to disallow the case. Anyhow, I think it would be preferable to specify a tzid for the start/endTime too.
There was a problem hiding this comment.
Hm, CalDateTime(2024, 10, 01) resolves to a DATE value, which cannot have a timezone (RFC 5545)?
There was a problem hiding this comment.
So you mean, it should always be a zoned DATE-TIME?
There was a problem hiding this comment.
Just a comment, not too relevant. Generally it would be advisable for users of the app, not to mix DATE vs DATE-TIME and floating vs non-floating, because there's always some ambiguity that causes additional complexity. In this case it really doesn't matter, so no need to change it. But as its advisable for users, not to mix it, we generally might want to avoid mixing them in the tests either (except for those cases where we explicitly want to test mixing them).
2 participants
ProcessRecurrencePattern.MatchTimeZoneto ensure UNTIL matches DTSTART's time zone and type following RFC 5545 definitionsCloses #663