Calendar time zone provider - #959
Open
maknapp wants to merge 9 commits into
Open
Conversation
Implements Nodatime's IDateTimeZoneProvider for Calendar using its VTIMEZONE components.
Evaluate events in custom time zone
This also stops caching the system time zone, which should not have been happening anyway because the system time zone can change.
Adds a case-insensitive time zone provider to more closely match previous behavior.
Events within a calendar can be evaluated in parallel, which would make multiple threads access TimeZoneProvider at the same time.
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (72.3%) is below the target coverage (80.0%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #959 +/- ##
=======================================
+ Coverage 71.5% 71.9% +0.5%
=======================================
Files 118 116 -2
Lines 4585 4698 +113
Branches 1068 1082 +14
=======================================
+ Hits 3277 3379 +102
- Misses 955 963 +8
- Partials 353 356 +3
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Collaborator
|
That sounds pretty nice. I'll need some time to go through changes. |
axunonb
reviewed
Jul 24, 2026
axunonb
left a comment
axunonb
left a comment
Collaborator
There was a problem hiding this comment.
This looks very well designed to me, great!
Nice fix to RECURRENCE-ID.
Few potential issues I came across. See the comments.
Sonar cloud comments: left to your discretion.
|
|
||
| var name = info.TimeZoneName ?? "Unknown"; | ||
|
|
||
| var wallOffset = Offset.FromTimeSpan(info.OffsetTo!.Offset); |
Collaborator
There was a problem hiding this comment.
Should this rather be this way? (subtract OffsetFrom)
Not covered by tests though. See suggested additional tests in VTimeZoneTest
var wallOffset = Offset.FromTimeSpan(info.OffsetTo!.Offset);
var savings = info.Name == Components.Standard
? Offset.Zero
: Offset.FromTimeSpan(info.OffsetTo!.Offset) - Offset.FromTimeSpan(info.OffsetFrom!.Offset);| { | ||
| if (data.TzId == null) | ||
| { | ||
| throw new Exception("Time zone ID must be set"); |
Collaborator
There was a problem hiding this comment.
Maybe introduce a specific VTimeZoneException?
If not use InvalidOperationException?
|
|
||
| if (info == null) | ||
| { | ||
| throw new Exception("Could not find zone interval"); |
Collaborator
There was a problem hiding this comment.
See above - VTimeZoneException?
| Assert.That(results, Has.Count.EqualTo(1)); | ||
| } | ||
|
|
||
| private static Calendar CreateTestCalendar(string tzId, DateTime? earliestTime = null, bool includeHistoricalData = true) |
Collaborator
There was a problem hiding this comment.
These additional tests fail with the original code in VTimeZone line 507-511
[Test, Category("VTimeZone")]
public void VTimeZone_CalendarDateTimeZone_GetZoneInterval_Should_Calculate_Savings_Correctly()
{
// Arrange: Create a calendar with a custom VTIMEZONE that has daylight saving time
var icalString = @"BEGIN:VCALENDAR
VERSION:2.0
PRODID:ical.net
BEGIN:VTIMEZONE
TZID:Test/DST
BEGIN:STANDARD
DTSTART:19701101T020000
RRULE:FREQ=YEARLY;BYMONTH=11;BYDAY=1SU
TZOFFSETFROM:-0400
TZOFFSETTO:-0500
TZNAME:EST
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:19700308T020000
RRULE:FREQ=YEARLY;BYMONTH=3;BYDAY=2SU
TZOFFSETFROM:-0500
TZOFFSETTO:-0400
TZNAME:EDT
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
DTSTART;TZID=Test/DST:20250615T120000
DTEND;TZID=Test/DST:20250615T130000
SUMMARY:Test Event
UID:test@example.com
END:VEVENT
END:VCALENDAR";
var calendar = Calendar.Load(icalString);
var vtz = calendar.TimeZones.First();
// Act: Get the DateTimeZone from the VTIMEZONE
var dateTimeZone = vtz.ToDateTimeZone();
// Get a zone interval during daylight saving time (June 2025)
var instantInDst = Instant.FromUtc(2025, 6, 15, 16, 0); // 12:00 EDT = 16:00 UTC
var dstInterval = dateTimeZone.GetZoneInterval(instantInDst);
// Get a zone interval during standard time (December 2025)
var instantInStd = Instant.FromUtc(2025, 12, 15, 17, 0); // 12:00 EST = 17:00 UTC
var stdInterval = dateTimeZone.GetZoneInterval(instantInStd);
// Assert: Daylight saving time should have non-zero savings
using (Assert.EnterMultipleScope())
{
// During daylight time (EDT), wall offset is -04:00
Assert.That(dstInterval.WallOffset.ToTimeSpan(), Is.EqualTo(TimeSpan.FromHours(-4)),
"Wall offset during DST should be -04:00");
// Savings should be +01:00 (the difference from standard time)
Assert.That(dstInterval.Savings.ToTimeSpan(), Is.EqualTo(TimeSpan.FromHours(1)),
"Savings during DST should be +01:00");
// Standard offset should be -05:00 (wall offset - savings)
Assert.That(dstInterval.StandardOffset.ToTimeSpan(), Is.EqualTo(TimeSpan.FromHours(-5)),
"Standard offset during DST should be -05:00");
// During standard time (EST), wall offset is -05:00
Assert.That(stdInterval.WallOffset.ToTimeSpan(), Is.EqualTo(TimeSpan.FromHours(-5)),
"Wall offset during standard time should be -05:00");
// Savings should be zero during standard time
Assert.That(stdInterval.Savings.ToTimeSpan(), Is.EqualTo(TimeSpan.Zero),
"Savings during standard time should be zero");
// Standard offset equals wall offset during standard time
Assert.That(stdInterval.StandardOffset.ToTimeSpan(), Is.EqualTo(TimeSpan.FromHours(-5)),
"Standard offset during standard time should be -05:00");
}
}
[Test, Category("VTimeZone")]
public void VTimeZone_CalendarDateTimeZone_GetZoneInterval_Savings_Should_Match_NodaTime_Tzdb()
{
// Arrange: Create a VTIMEZONE from a known TZDB zone with daylight saving time
var tzId = "America/New_York";
var vtz = VTimeZone.FromDateTimeZone(tzId);
var calendar = new Calendar();
calendar.AddChild(vtz);
var calendarDateTimeZone = vtz.ToDateTimeZone();
var tzdbZone = CalendarTimeZoneProviders.TzdbWithAliases[tzId];
// Act & Assert: Compare savings during both DST and standard time
var testInstants = new[]
{
Instant.FromUtc(2025, 7, 15, 12, 0), // Mid-summer (DST active)
Instant.FromUtc(2025, 1, 15, 12, 0) // Mid-winter (standard time)
};
foreach (var instant in testInstants)
{
var tzdbInterval = tzdbZone.GetZoneInterval(instant);
var calendarInterval = calendarDateTimeZone.GetZoneInterval(instant);
using (Assert.EnterMultipleScope())
{
Assert.That(calendarInterval.WallOffset, Is.EqualTo(tzdbInterval.WallOffset),
$"Wall offset mismatch at {instant}");
Assert.That(calendarInterval.Savings, Is.EqualTo(tzdbInterval.Savings),
$"Savings mismatch at {instant}: Expected {tzdbInterval.Savings}, but got {calendarInterval.Savings}");
Assert.That(calendarInterval.StandardOffset, Is.EqualTo(tzdbInterval.StandardOffset),
$"Standard offset mismatch at {instant}");
}
}
}| public string VersionId => field ??= "combined: " + string.Join(", ", providers.Select(x => x.VersionId)); | ||
|
|
||
|
|
||
| public DateTimeZone GetSystemDefault() => throw new NotImplementedException(); |
Collaborator
There was a problem hiding this comment.
Maybe implement e.g. like this?
public DateTimeZone GetSystemDefault()
{
return providers.FirstOrDefault()?.GetSystemDefault()
?? DateTimeZoneProviders.Tzdb.GetSystemDefault();
}| protected override NodaTime.ZonedDateTime GetEnd(NodaTime.ZonedDateTime start) => start; | ||
|
|
||
| protected override NodaTime.IDateTimeZoneProvider TimeZoneProvider => CalendarTimeZoneProviders.TzdbWithAliases; | ||
|
|
Collaborator
There was a problem hiding this comment.
Would it make sense to implement like this?
protected override EvaluationPeriod EvaluateRDate(Period rdate, NodaTime.DateTimeZone referenceTimeZone)
{
var start = rdate.StartTime.ToZonedOrDefault(referenceTimeZone, TimeZoneProvider);
var end = GetEnd(start);
return new EvaluationPeriod(start, end);
}VTimeZoneInfo is used to represent STANDARD and DAYLIGHT components within a VTIMEZONE
(Time zone transitions are instants, not periods with duration.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a
Calendar.TimeZoneProviderproperty to allow users to specify theNodaTime.IDateTimeZoneProviderto use during calendar/event evaluation. The default time zone provider for a calendar isCalendarTimeZoneProviders.TzdbWithAliaseswith the calendar defined time zones as the fallback. The default time zone provider for components without a calendar parent isCalendarTimeZoneProviders.TzdbWithAliases.User choice
Users can choose the time zone provider:
CalendarTimeZoneProviders.Combine()to create their ownNodaTime.IDateTimeZoneProviderthat searches the specified providers in the given order for a matching time zoneNodaTime'sDateTimeZoneProviders.Tzdbdirectly - with or without the calendar's VTIMEZONE componentscal.TimeZoneProvider = cal.CreateTimeZoneProvider())VTIMEZONE
VTIMEZONE changes are not automatically handled. VTIMEZONE components are converted to
NodaTime.DateTimeZonevalues on demand and cached. If a VTIMEZONE component is added/removed/changed AFTER being used by the time zone provider, theCalendar.TimeZoneProviderwill need to be recreated to be up-to-date with the calendar VTIMEZONE components.One (unlikely?) issue users might run into is that
NodaTime.ZonedDateTimechecks equality byReferenceEqualsof itsDateTimeZone, so recreating theCalendar.TimeZoneProvidercould cause issues if the user tries to compare aZonedDateTimewith another created from a different time zone provider because they would have two differentDateTimeZoneobjects even though they represent the same VTIMEZONE.Exceptions
Missing time zone errors will now throw NodaTime's DateTimeZoneNotFoundException instead of ArgumentException. I'm not sure if there are downsides vs catching and rethrowing with our own
TimeZoneNotFoundException. I do like having a more specific exception instead ofArgumentException. Related #648.No more time zone exceptions in Calendar.Load(). As stated above, exceptions due to missing time zones are now thrown during evaluation.
Time zone searching
The
Calendar.TimeZoneProvideris created on demand with a default value if none is specifically set. The default is NOT an exact copy of the previousTimeZoneResolver. Since this brings more time zone control to the user, it seemed reasonable to remove the more non-standard searching by default: case-insensitive andstring.Containssearching. I did add a case-insensitive time zone provider in case that is actually needed - I would like to remove that commit though unless users actually need it. I ignored adding astring.Containsreplacement because that seems very inaccurate to me.Searching
XmlSerializationSettingswas also removed completely because it is, by NodaTime default, just the TZDB. What is the purpose of searchingNodaTime.Xml.XmlSerializationSettings.DateTimeZoneProvider? The user can add this provider if they actually use this, so I think it is safe to ignore it completely.Global time zone handling removed
DefaultTimeZoneResolverandTimeZoneResolverswere removed. Having a default time zone provider on the calendar is essentially the same thing for users who currently ignore time zones.CalDateTime
This now requires a time zone provider to convert to a
ZonedDateTime. @axunonb mentioned in #749 that there should be a resolver-less overload still, but a user converting to aZonedDateTimeshould always be paying attention to the time zone provider anyway. If a user really wants to ignore it, they can just use extensions to use a default time zone provider.ToDateTimeUtc()is also removed just because I did not want to require aNodaTimetime zone provider to create aDateTime. Users avoidingNodaTimecan still useToDateTimeUnspecified()to get just the local time. Users can also use extensions to create aToDateTimeUtc()however they want.None of these changes are breaking changes for
CalDateTimebecause they are not in v5.Fixes #749