ical-org/ical.net

`RecurrencePattern.Until` should be of type `IDateTime` instead of `DateTime`

Opened this issue · 2 comments

Reasoning:
In the RecurrencePatternSerializer the RecurrencePattern.Until is necessarily converted to an IDateTime for serialization.
That's why it would be better to store it as an IDateTime from the beginning, instead of relying on RecurrencePattern.Until.Kind.

if (recur.Until != DateTime.MinValue)
{
    var serializer = factory.Build(typeof(IDateTime), SerializationContext) as IStringSerializer;
    if (serializer != null)
    {
        IDateTime until = new CalDateTime(recur.Until); // <<<<<<<<< The DateTimeKind is important here
        until.HasTime = true;
        values.Add("UNTIL=" + serializer.SerializeToString(until));
    }
}

Generally, using DateTime is a break in how date/time is represented throughout the library. In most (all?) other places, a date/time that represents a value from the RFC is implemented as IDateTime. Its not obvious, why this is different in the case of UNTIL. A reason might be, that UNTIL most often needs to be represented in UTC and sometimes as floating time (if DTSTART is floating), but never with timezone reference, so at first glance DateTime might seem sufficient. Nevertheless IDateTime aka CalDateTime would be the more natural fit, because

  • it can represent floating time vs UTC
  • it can represent DATE vs DATE-TIME
  • it is the way it is done throughout the lib
  • it doesn't have the ambiguity of DateTime, e.g. regarding DateTimeKind.Local.

While serialization works (renders date/time as UTC), we can apply a fix to get the deserialized UTC value correctly
by replacing

if (dt != null)
{
r.Until = dt.Value;
}

with

r.Until = DateTime.SpecifyKind(dt.Value, dt.IsUtc ? DateTimeKind.Utc : DateTimeKind.Unspecified);

But it still means to rely on DateTimeKind.
This might also fix #406 at least partly.