Zoned occurrence using NodaTime#854
Conversation
|
Wow, this PR is huge and needs some time to review. In general this closer move towards [Edit] v5.1.0
Current PR (Commit 0a6a55e, Store CalDateTime values with NodaTime)
Current PR (Commit 3394bcd, Reduce evaluation allocation)Reduce evaluation allocation
|
maknapp
force-pushed
the
zoned-occurrence
branch
from
b30d995 to
3394bcd
Compare
|
I was considering this when I started and ended up changing the public API to NodaTime just to make everything how I would want to use it, and then cut back rejected changes later. Arguments for public NodaTime is that NodaTime contains time zone data, so using multiple NodaTime versions would lead to time zone database mismatches. To remove NodaTime, the public API could be changed to something like below: IEnumerable<Occurrence> GetOccurrences(string timeZone, DateTimeOffset? startTime = null);
public class Occurrence : IComparable<Occurrence>
{
public IRecurrable Source { get; private set; }
public DateTimeOffset Start { get; private set; }
public DateTimeOffset End { get; private set; }
public string TimeZoneId { get; private set; }
}
All |
|
@maknapp Apologies for the delay in getting back to your PR. I want to acknowledge the significant effort you've put into this—great work, and thank you! There are a few points that you, @minichma and I should discuss. This list is by no means exhaustive, but it’s a starting point for further conversation:
@maknapp @minichma I'd love for both of you to join the discussion around these points and others that come to your mind. Your insights will be invaluable as we shape the direction for ical.net v6. Let's collaborate to make the transition smooth, maintainable and future-proof. |
Most changes in this pull request do not change the general logic/structure of evaluation, so hopefully bringing in v5 changes will involve mostly just changing types to fit.
For your specific example,
It may not be very helpful keeping the various |
|
I'm trying to figure out with which commit the evaluation of ambiguous or non-existing dates started to work correctly, e.g. with DST changes. Was it with ba75ef5? Here is the API Diff from v5.1.1 vs PR854API diff: Ical.Net.dllIcal.Net.dll
Namespace Ical.NetType Changed: Ical.Net.CalendarRemoved methods: public virtual CalendarComponents.FreeBusy GetFreeBusy (CalendarComponents.FreeBusy freeBusyRequest);
public virtual CalendarComponents.FreeBusy GetFreeBusy (DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public virtual CalendarComponents.FreeBusy GetFreeBusy (DataTypes.Organizer organizer, System.Collections.Generic.IEnumerable<DataTypes.Attendee> contacts, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);Added methods: public virtual CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, CalendarComponents.FreeBusy freeBusyRequest);
public virtual CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public virtual CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, DataTypes.Organizer organizer, System.Collections.Generic.IEnumerable<DataTypes.Attendee> contacts, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (NodaTime.ZonedDateTime startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.ZonedDateTime startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (NodaTime.DateTimeZone tz, NodaTime.Instant? startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.DateTimeZone tz, NodaTime.Instant? startTime, Evaluation.EvaluationOptions options);Type Changed: Ical.Net.CalendarCollectionRemoved methods: public CalendarComponents.FreeBusy GetFreeBusy (CalendarComponents.FreeBusy freeBusyRequest);
public CalendarComponents.FreeBusy GetFreeBusy (DataTypes.Organizer organizer, System.Collections.Generic.IEnumerable<DataTypes.Attendee> contacts, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);
public System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);Added methods: public CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, CalendarComponents.FreeBusy freeBusyRequest);
public CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, DataTypes.Organizer organizer, System.Collections.Generic.IEnumerable<DataTypes.Attendee> contacts, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.ZonedDateTime startTime, Evaluation.EvaluationOptions options);
public System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, Evaluation.EvaluationOptions options);Type Changed: Ical.Net.CollectionExtensionsRemoved method: public static System.Collections.Generic.IEnumerable<DataTypes.Period> TakeWhileBefore (this System.Collections.Generic.IEnumerable<DataTypes.Period> sequence, DataTypes.CalDateTime periodEnd);Obsoleted methods: [Obsolete ("Use NodaTime.Instant to specify period end.")]
public static System.Collections.Generic.IEnumerable<DataTypes.Occurrence> TakeWhileBefore (this System.Collections.Generic.IEnumerable<DataTypes.Occurrence> sequence, DataTypes.CalDateTime periodEnd);Added methods: public static System.Collections.Generic.IEnumerable<DataTypes.Occurrence> TakeWhileBefore (this System.Collections.Generic.IEnumerable<DataTypes.Occurrence> sequence, NodaTime.Instant periodEnd);
public static System.Collections.Generic.IEnumerable<Evaluation.EvaluationPeriod> TakeWhileBefore (this System.Collections.Generic.IEnumerable<Evaluation.EvaluationPeriod> sequence, NodaTime.Instant periodEnd);Type Changed: Ical.Net.IGetFreeBusyRemoved methods: public virtual CalendarComponents.FreeBusy GetFreeBusy (CalendarComponents.FreeBusy freeBusyRequest);
public virtual CalendarComponents.FreeBusy GetFreeBusy (DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public virtual CalendarComponents.FreeBusy GetFreeBusy (DataTypes.Organizer organizer, System.Collections.Generic.IEnumerable<DataTypes.Attendee> contacts, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);Added methods: public virtual CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, CalendarComponents.FreeBusy freeBusyRequest);
public virtual CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);
public virtual CalendarComponents.FreeBusy GetFreeBusy (NodaTime.DateTimeZone timeZone, DataTypes.Organizer organizer, System.Collections.Generic.IEnumerable<DataTypes.Attendee> contacts, DataTypes.CalDateTime fromInclusive, DataTypes.CalDateTime toExclusive);Type Changed: Ical.Net.IGetOccurrencesRemoved method: public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);Added methods: public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.ZonedDateTime startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, Evaluation.EvaluationOptions options);Type Changed: Ical.Net.IGetOccurrencesTypedRemoved method: public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);Added methods: public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (NodaTime.ZonedDateTime startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences<T> (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, Evaluation.EvaluationOptions options);Type Changed: Ical.Net.VTimeZoneInfoRemoved property: public virtual DataTypes.RecurrenceIdentifier RecurrenceIdentifier { get; set; }Removed method: public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (DataTypes.CalDateTime startTime, Evaluation.EvaluationOptions options);Added methods: public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.ZonedDateTime startTime, Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<DataTypes.Occurrence> GetOccurrences (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, Evaluation.EvaluationOptions options);Removed Type Ical.Net.CalendarExtensionsNamespace Ical.Net.CalendarComponentsType Changed: Ical.Net.CalendarComponents.AlarmRemoved methods: public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> GetOccurrences (IRecurringComponent rc, Ical.Net.DataTypes.CalDateTime fromDate, Ical.Net.Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> Poll (Ical.Net.DataTypes.CalDateTime start, Ical.Net.Evaluation.EvaluationOptions options);Added methods: public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> GetOccurrences (IRecurringComponent rc, NodaTime.DateTimeZone timeZone, NodaTime.Instant? fromDate, Ical.Net.Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> Poll (NodaTime.DateTimeZone timeZone, NodaTime.Instant? start, Ical.Net.Evaluation.EvaluationOptions options);Type Changed: Ical.Net.CalendarComponents.CalendarEventRemoved interface: System.IComparable<CalendarEvent>Removed property: public Ical.Net.DataTypes.Duration EffectiveDuration { get; }Removed method: public virtual int CompareTo (CalendarEvent other);Type Changed: Ical.Net.CalendarComponents.FreeBusyRemoved method: public static FreeBusy Create (Ical.Net.ICalendarObject obj, FreeBusy freeBusyRequest, Ical.Net.Evaluation.EvaluationOptions options);Added methods: public static FreeBusy Create (Ical.Net.ICalendarObject obj, NodaTime.DateTimeZone timeZone, FreeBusy freeBusyRequest, Ical.Net.Evaluation.EvaluationOptions options);
public virtual Ical.Net.FreeBusyStatus GetFreeBusyStatus (NodaTime.Instant? dt);Type Changed: Ical.Net.CalendarComponents.IAlarmContainerRemoved method: public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> PollAlarms (Ical.Net.DataTypes.CalDateTime startTime, Ical.Net.DataTypes.CalDateTime endTime);Added method: public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> PollAlarms (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, NodaTime.Instant? endTime);Type Changed: Ical.Net.CalendarComponents.RecurringComponentRemoved property: public virtual Ical.Net.DataTypes.RecurrenceIdentifier RecurrenceIdentifier { get; set; }Removed methods: public virtual System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Occurrence> GetOccurrences (Ical.Net.DataTypes.CalDateTime startTime, Ical.Net.Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> PollAlarms ();
public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> PollAlarms (Ical.Net.DataTypes.CalDateTime startTime, Ical.Net.DataTypes.CalDateTime endTime);Added methods: public virtual System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Occurrence> GetOccurrences (NodaTime.ZonedDateTime startTime, Ical.Net.Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Occurrence> GetOccurrences (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, Ical.Net.Evaluation.EvaluationOptions options);
public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> PollAlarms (NodaTime.DateTimeZone timeZone);
public virtual System.Collections.Generic.IList<Ical.Net.DataTypes.AlarmOccurrence> PollAlarms (NodaTime.DateTimeZone timeZone, NodaTime.Instant? startTime, NodaTime.Instant? endTime);Type Changed: Ical.Net.CalendarComponents.TodoRemoved property: public Ical.Net.DataTypes.Duration? EffectiveDuration { get; }Removed methods: public virtual bool IsActive (Ical.Net.DataTypes.CalDateTime currDt);
public virtual bool IsCompleted (Ical.Net.DataTypes.CalDateTime currDt);Added methods: public virtual bool IsActive (NodaTime.ZonedDateTime value);
public virtual bool IsCompleted (NodaTime.ZonedDateTime currDt);Namespace Ical.Net.DataTypesType Changed: Ical.Net.DataTypes.AlarmOccurrenceRemoved constructor: public AlarmOccurrence (Ical.Net.CalendarComponents.Alarm a, CalDateTime dt, Ical.Net.CalendarComponents.IRecurringComponent rc);Added constructor: public AlarmOccurrence (Ical.Net.CalendarComponents.Alarm a, NodaTime.ZonedDateTime start, Ical.Net.CalendarComponents.IRecurringComponent rc);Removed properties: public CalDateTime DateTime { get; set; }
public Period Period { get; set; }Modified properties: public Ical.Net.CalendarComponents.Alarm Alarm { get; ---set;--- }
public Ical.Net.CalendarComponents.IRecurringComponent Component { get; ---set;--- }Added property: public NodaTime.ZonedDateTime Start { get; }Type Changed: Ical.Net.DataTypes.CalDateTimeRemoved constructor: public CalDateTime (CalDateTime value);Added constructors: public CalDateTime (NodaTime.Instant instant);
public CalDateTime (NodaTime.LocalDate date);
public CalDateTime (NodaTime.LocalDate value, string tzId);
public CalDateTime (NodaTime.LocalDateTime value, string tzId);
public CalDateTime (NodaTime.LocalDate date, NodaTime.LocalTime? time, string tzId);Removed interface: System.IComparable<CalDateTime>Modified properties: -public System.DateOnly Date { get; }
+public NodaTime.LocalDate Date { get; }
-public System.TimeOnly? Time { get; }
+public NodaTime.LocalTime? Time { get; }Added properties: public System.DateOnly DateOnly { get; }
public System.TimeOnly? TimeOnly { get; }Removed methods: public virtual int CompareTo (CalDateTime dt);
public bool GreaterThan (CalDateTime dt);
public bool GreaterThanOrEqual (CalDateTime dt);
public bool LessThan (CalDateTime dt);
public bool LessThanOrEqual (CalDateTime dt);
public Duration Subtract (CalDateTime dt);
public System.TimeSpan SubtractExact (CalDateTime dt);
public static bool op_GreaterThan (CalDateTime left, CalDateTime right);
public static bool op_GreaterThanOrEqual (CalDateTime left, CalDateTime right);
public static bool op_LessThan (CalDateTime left, CalDateTime right);
public static bool op_LessThanOrEqual (CalDateTime left, CalDateTime right);Added methods: public NodaTime.ZonedDateTime AsZonedOrDefault (NodaTime.DateTimeZone timeZone);
public NodaTime.Instant ToInstant ();
public NodaTime.LocalDateTime ToLocalDateTime ();
public NodaTime.ZonedDateTime ToZonedDateTime ();
public NodaTime.ZonedDateTime ToZonedDateTime (NodaTime.DateTimeZone timeZone);
public NodaTime.ZonedDateTime ToZonedDateTime (string zoneId);Type Changed: Ical.Net.DataTypes.DurationRemoved method: public System.TimeSpan ToTimeSpan (CalDateTime start);Added methods: public NodaTime.Period GetNominalPart ();
public NodaTime.Duration GetTimePart ();
public NodaTime.Period ToPeriod ();Type Changed: Ical.Net.DataTypes.FreeBusyEntryRemoved interface: System.IComparable<Period>Added methods: public bool CollidesWith (Period period);
public bool Contains (CalDateTime dt);
public bool Contains (NodaTime.Instant value);Type Changed: Ical.Net.DataTypes.OccurrenceRemoved constructor: public Occurrence (Ical.Net.CalendarComponents.IRecurrable recurrable, Period period);Added constructor: public Occurrence (Ical.Net.CalendarComponents.IRecurrable recurrable, NodaTime.ZonedDateTime start, NodaTime.ZonedDateTime end);Modified properties: -public Period Period { get; ---set;--- }
+public System.ValueTuple<NodaTime.ZonedDateTime,NodaTime.ZonedDateTime> Period { get; set; }
public Ical.Net.CalendarComponents.IRecurrable Source { get; ---set;--- }Added properties: public CalDateTime DtStart { get; }
public NodaTime.ZonedDateTime End { get; }
public NodaTime.ZonedDateTime Start { get; }Added method: public bool Contains (CalDateTime value);Type Changed: Ical.Net.DataTypes.PeriodRemoved interface: System.IComparable<Period>Removed properties: public virtual Duration? EffectiveDuration { get; }
public virtual CalDateTime EffectiveEndTime { get; }Added property: public bool HasEndOrDuration { get; }Removed methods: public virtual bool CollidesWith (Period period);
public virtual int CompareTo (Period other);
public virtual bool Contains (CalDateTime dt);Removed Type Ical.Net.DataTypes.RecurrenceIdentifierRemoved Type Ical.Net.DataTypes.RecurrenceRangeNamespace Ical.Net.EvaluationType Changed: Ical.Net.Evaluation.EvaluatorRemoved methods: public virtual System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);
protected void IncrementDate (ref Ical.Net.DataTypes.CalDateTime dt, Ical.Net.DataTypes.RecurrencePattern pattern, int interval);Added methods: public virtual System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.ZonedDateTime periodStart, EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.DateTimeZone timeZone, NodaTime.Instant? periodStart, EvaluationOptions options);
protected static void IncrementDate (ref NodaTime.ZonedDateTime dt, Ical.Net.DataTypes.RecurrencePattern pattern, int interval);Type Changed: Ical.Net.Evaluation.EventEvaluatorRemoved property: protected override Ical.Net.DataTypes.Duration? DefaultDuration { get; }Removed method: public override System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);Added methods: protected override EvaluationPeriod EvaluateRDate (Ical.Net.DataTypes.Period rdate, NodaTime.DateTimeZone referenceTimeZone);
protected override NodaTime.ZonedDateTime GetEnd (NodaTime.ZonedDateTime start);Type Changed: Ical.Net.Evaluation.IEvaluatorRemoved method: public virtual System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);Added methods: public virtual System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.ZonedDateTime periodStart, EvaluationOptions options);
public virtual System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.DateTimeZone timeZone, NodaTime.Instant? periodStart, EvaluationOptions options);Type Changed: Ical.Net.Evaluation.RecurrencePatternEvaluatorRemoved method: public override System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);Added method: public override System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.DateTimeZone timeZone, NodaTime.Instant? periodStart, EvaluationOptions options);Type Changed: Ical.Net.Evaluation.RecurringEvaluatorRemoved property: protected virtual Ical.Net.DataTypes.Duration? DefaultDuration { get; }Removed methods: public override System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);
protected System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> EvaluateRDate ();
protected System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> EvaluateRRule (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);Added methods: public override System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.DateTimeZone timeZone, NodaTime.Instant? periodStart, EvaluationOptions options);
protected System.Collections.Generic.SortedSet<EvaluationPeriod> EvaluateRDate (NodaTime.DateTimeZone referenceTimeZone);
protected virtual EvaluationPeriod EvaluateRDate (Ical.Net.DataTypes.Period rdate, NodaTime.DateTimeZone referenceTimeZone);
protected System.Collections.Generic.IEnumerable<EvaluationPeriod> EvaluateRRule (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.DateTimeZone timeZone, NodaTime.Instant? periodStart, EvaluationOptions options);
protected virtual NodaTime.ZonedDateTime GetEnd (NodaTime.ZonedDateTime start);Type Changed: Ical.Net.Evaluation.TimeZoneInfoEvaluatorRemoved property: protected override Ical.Net.DataTypes.Duration? DefaultDuration { get; }Added methods: protected override EvaluationPeriod EvaluateRDate (Ical.Net.DataTypes.Period rdate, NodaTime.DateTimeZone referenceTimeZone);
protected override NodaTime.ZonedDateTime GetEnd (NodaTime.ZonedDateTime start);Type Changed: Ical.Net.Evaluation.TodoEvaluatorRemoved property: protected override Ical.Net.DataTypes.Duration? DefaultDuration { get; }Removed method: public override System.Collections.Generic.IEnumerable<Ical.Net.DataTypes.Period> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, Ical.Net.DataTypes.CalDateTime periodStart, EvaluationOptions options);Added methods: public override System.Collections.Generic.IEnumerable<EvaluationPeriod> Evaluate (Ical.Net.DataTypes.CalDateTime referenceDate, NodaTime.DateTimeZone timeZone, NodaTime.Instant? periodStart, EvaluationOptions options);
protected override EvaluationPeriod EvaluateRDate (Ical.Net.DataTypes.Period rdate, NodaTime.DateTimeZone referenceTimeZone);
protected override NodaTime.ZonedDateTime GetEnd (NodaTime.ZonedDateTime start);New Type: Ical.Net.Evaluation.EvaluationPeriodpublic class EvaluationPeriod : System.IComparable<EvaluationPeriod> {
// constructors
public EvaluationPeriod (NodaTime.ZonedDateTime start, NodaTime.ZonedDateTime? end);
// properties
public NodaTime.ZonedDateTime? End { get; }
public NodaTime.ZonedDateTime Start { get; }
// methods
public virtual int CompareTo (EvaluationPeriod other);
public bool Equals (EvaluationPeriod other);
public override bool Equals (object obj);
public override int GetHashCode ();
public EvaluationPeriod WithZone (NodaTime.DateTimeZone zone);
}Namespace Ical.Net.Serialization.DataTypesRemoved Type Ical.Net.Serialization.DataTypes.RecurrenceIdentifierSerializer |
|
ba75ef5 does evaluation similar to how it was before, where time zone is considered only at the end of evaluation instead of at every step. I believe most of the original tests pass with this commit too. Ambiguous time change was fixed in the next commit, c42f14a (see commit description, 2nd paragraph). This commit changes all evaluation so that time zone is considered everywhere. Compare Evaluator.cs differences between these two commits - A lot of the API diff is adding |
The ReviewI reviewed all commits to understand the rationale behind the design decisions made throughout the implementation process. The commit history shows a design that evolved as the complexities of The Proposed DesignThe critical problem resolved is regarding evaluation: The standard .NET date and time types ( The Mixed Model ProblemEven if we fix the time zone bugs by using The question that came to mind: would the final design have looked the same if the solution had been known from the beginning? A "greenfield" design, knowing from the start that a pure separation was the goal, would look different: Regarding the separation of the iCalendar data model ( A Clean Separation Design
|
|
@axunonb Great summary! The questions you ask are quite fundamental and I can't answer them just add my 2c. The problems that come with today's architecture are quite subtle and show up only in edge cases like ambiguous times during DST changes. I don't think that a relevant number of users have ever had any issues with it. However, the existence of those problems show a fundamental shortcoming of the solution and on the long run it would certainly good to have the architecture fixed. But whether its worth it really depends on the ambitions of the maintainers. As mentioned earlier, I personally won't be able to contribute much to a next version. Regarding the concrete architecture: I fully agree that today's model types shouldn't be polluted with Noda types because it would make things even more complex (I didn't review much of this PR though so my comments are rather generic). Besides leaving everything as is or doing a full rewrite, there could also be a way in between. I.e. the type causing most issues so far is the So long story short, a full rework of the evaluation part would be great if there are people (mostly you guys) that have the ambition to get that done. Alternatively some smaller 'fire fighting' approach (i.e. introducing a new type for returning occurrences) could bring quite some improvement too. The latter could also be implemented as intermediate step before taking on the big ones. |
Yes.
I did look through that pull request as I was starting to work through what I wanted to change.
This was my plan but I ended up leaving a lot of methods just to reduce changes for tests until we could discuss. While making
I'm not sure if you are saying to only store End or Duration but not both? To be clear, Period needs both End and Duration properties. One reason is for deserialize -> serialize equality. Another reason is that start/end is absolute while start/duration is not. From the spec, "For example, recurrence instances of a nominal duration of one day will have an exact duration of more or less than 24 hours on a day where a time zone shift occurs."
I think I agree with this? I need to go back and look through everything again, but I remember having questions about that area. Since these changes will require changing a lot of tests, changing the test framework from NUnit to XUnit should be considered. I personally think XUnit is easier to use, but some tests use NUnit features that XUnit does not have (or handles differently). I am no expert on either one, so if there is a strong opinion on this, please share. |
Yes, of course. I had
Hard to say. For sure these methods are very convenient while working with ical.net, that's why we see heavy usage in the unit tests.
As long as we understand
NUnit is a mature and widely adopted framework, and it’s already well-integrated into our test infrastructure. Introducing a new test runner would mean retooling CI, rewriting attributes, and retraining contributors - none of which feels justified right now. If the goal is cleaner assertions, Shouldly works great alongside NUnit and doesn’t require structural changes. |
| /// </remarks> | ||
| /// </summary> | ||
| public sealed class CalDateTime : IComparable<CalDateTime>, IFormattable | ||
| public sealed class CalDateTime : IFormattable |
There was a problem hiding this comment.
Finally this should become a small, dump iCalendar wall time representation. Wall time arithmetic could go into extension methods.
| /// Gets the day. | ||
| /// </summary> | ||
| public int Year => Value.Year; | ||
| public int Day => _localDate.Day; |
There was a problem hiding this comment.
Do the wrapped Day, Month, Year, Hour etc. properties bring a benefit to users?
Exposing DateTime could be sufficient?
There was a problem hiding this comment.
They are useful to have and simple enough to keep. They are used significantly in tests. Anyone using CalDateTime directly would probably prefer if (calDateTime.Year == 2025) over if (calDateTime.Date.Year == 2025). For comparison, NodaTime types provide these wrapped properties everywhere.
The time values being nullable vs defaulting to 0 needs some thought though. Nullable is more accurate. Defaulting to 0 matches the values of ToLocalDateTime() and Value.
| public int DayOfYear => Value.DayOfYear; | ||
| public LocalTime? Time => _localTime; | ||
|
|
||
| #if NET6_0_OR_GREATER |
There was a problem hiding this comment.
The pragma is okay for the CTOR, but should public properties be different across target frameworks?
There was a problem hiding this comment.
I changed them to methods ToDateOnly() and ToTimeOnly() to match NodaTime. Is there a specific argument against this?
There was a problem hiding this comment.
Yes, it would be good to have the same public API for all target frameworks. The comment is about the #if NET6_0_OR_GREATER pragma influencing the public API.
As the pkg Portable.System.DateTimeOnly is removed and internally replaced by NodaTime types, what would be the purpose for TimeOnly / DateOnly? So I'd vote for dropping TimeOnly and DateOnly altogether.
| return TimeAdjusters.TruncateToSecond(time.Value); | ||
| } | ||
|
|
||
| public LocalDateTime ToLocalDateTime() | ||
| { | ||
| var localDate = new LocalDate(_localDate.Year, _localDate.Month, _localDate.Day); | ||
|
|
||
| if (_localTime is null) | ||
| { | ||
| return localDate.AtMidnight(); | ||
| } | ||
| else | ||
| { | ||
| var time = _localTime.Value; | ||
| return localDate.At(new LocalTime(time.Hour, time.Minute, time.Second)); | ||
| } | ||
| } | ||
|
|
||
| public Instant ToInstant() => ToZonedDateTime().ToInstant(); | ||
|
|
||
| public ZonedDateTime ToZonedDateTime() | ||
| { | ||
| if (_tzId is null) | ||
| { | ||
| return ToLocalDateTime().InUtc(); | ||
| } | ||
| else | ||
| { | ||
| return DateUtil.GetZone(_tzId).AtLeniently(ToLocalDateTime()); | ||
| } | ||
| } | ||
|
|
||
| public ZonedDateTime ToZonedDateTime(DateTimeZone timeZone) | ||
| { | ||
| if (_tzId is null) | ||
| { | ||
| return ToLocalDateTime().InZoneLeniently(timeZone); | ||
| } | ||
| else | ||
| { | ||
| return DateUtil.GetZone(_tzId) | ||
| .AtLeniently(ToLocalDateTime()) | ||
| .WithZone(timeZone); | ||
| } | ||
| } | ||
|
|
||
| public ZonedDateTime ToZonedDateTime(string zoneId) | ||
| { | ||
| return ToZonedDateTime(DateUtil.GetZone(zoneId)); | ||
| } | ||
|
|
||
| public ZonedDateTime AsZonedOrDefault(DateTimeZone timeZone) | ||
| { | ||
| if (_tzId is null) | ||
| { | ||
| return ToLocalDateTime().InZoneLeniently(timeZone); | ||
| } | ||
| else | ||
| { | ||
| return DateUtil.GetZone(_tzId).AtLeniently(ToLocalDateTime()); | ||
| } |
There was a problem hiding this comment.
For evaluation we could use an internal class EvaluationDateTime or alike. NodaTime types cannot cover all iCalendar evaluation rules.
| Seconds = seconds; | ||
| } | ||
|
|
||
| public NodaTime.Period ToPeriod() |
There was a problem hiding this comment.
Change members returning NodaTime.Period to internal?
| /// To convert a duration to a <see cref="TimeSpan"/> while considering the days and weeks as nominal durations, | ||
| /// use <see cref="ToTimeSpan"/>. | ||
| /// </remarks> | ||
| public TimeSpan ToTimeSpan(CalDateTime start) |
|
|
||
| if (EndTime is null && Duration is null) | ||
| { | ||
| throw new ArgumentException("Period must have a duration", nameof(period)); |
There was a problem hiding this comment.
"Period must either have a Duration or an EndTime"?
Or should we create a FreeBusyEntry from EvaluationPeriod?
There was a problem hiding this comment.
I fixed this error message. FreeBusyEntry is Period with a FBTYPE (FreeBusyStatus) and the requirement that datetimes MUST be UTC. Nothing verifies that dates are UTC here when it probably should.
| public Period Period { get; set; } | ||
| public IRecurrable Source { get; set; } | ||
| public IRecurrable Source { get; private set; } | ||
| public ZonedDateTime Start { get; private set; } |
There was a problem hiding this comment.
Expose ZonedDateTime for Start and End, or the wall time (CalDateTime) - do we need both?
Make other members internal?
There was a problem hiding this comment.
NodaTime.ZonedDateTime has all the data involved and is what is used while evaluating. Other types like NodaTime.Instant or NodaTime.OffsetDateTime can be used, but time zone would be lost. Since time zone is known already (because GetOccurrences() requires it), it is not unreasonable to drop the time zone and assume that the user knows that ALL occurrences are in the given time zone. CalDateTime can represent an exact time, but it is ambiguous so not very helpful. IF we want to hide NodaTime completely, using DateTimeOffset for Start and End would be the most accurate option.
|
|
||
| if (end != null && end.LessThan(start)) | ||
| throw new ArgumentException($"End time ({end}) must be greater than start time ({start}).", nameof(end)); | ||
| if (end != null) |
There was a problem hiding this comment.
Make Period as dump as CalDateTime - a pure iCalendar duration without evaluation?
| public ZonedDateTime? End { get; private set; } | ||
|
|
||
| public EvaluationPeriod(ZonedDateTime start, ZonedDateTime? end = null) | ||
| { |
There was a problem hiding this comment.
Move logic that currently exist in Period, e.g. start >= end etc.?
| using NodaTime; | ||
|
|
||
| namespace Ical.Net.Evaluation; | ||
| public class EvaluationPeriod : IComparable<EvaluationPeriod> |
There was a problem hiding this comment.
Could all Evaluation classes become internal. Would increase flexibility for future changes without causing a breaking change?
There was a problem hiding this comment.
I'm not sure. Some people might be using RecurrencePatternEvaluator directly for expanding RRULEs without using calendar features - similar to using rrule.js. Forcing the use of Calendar would have more allocations and probably be slightly slower.
| using NodaTime.TimeZones; | ||
|
|
||
| namespace Ical.Net; | ||
| internal static class NodaTimeExtensions |
|
|
||
| var datePart = new DateOnly(); // Initialize. At this point, we know that the date part is present | ||
| TimeOnly? timePart = null; | ||
| var datePart = new LocalDate(); // Initialize. At this point, we know that the date part is present |
|
I will get to your line comments above soon and see about fixing the remaining tests. When we are happy with test changes, we can move this over to the v6 branch. I have been working on a replacement |
The few tests still failing were left alone on purpose. With some tests, the test data needs to be changed because it does not make sense now that all occurrences are in a specific time zone. For FREEBUSY tests, that was mostly ignored because I am not sure how it should be used.
The only valid comparison for two CalDateTime values is equality. The greater and less than operators cannot always return a valid result. This makes implementing IComparable also invalid. "20250815" != "20250815T000000" but these operators also did not say they were less or greater either.
CalDateTime is immutable, so there is no need to copy.
CalDateTime was validating year using DateTime limits. NodaTime supports more than just positive years, so verify that the year is positive to match DateTime behavior.
This test had an effective end that exceeded the valid date range but only actually held a duration, so there was no exception. The new occurrence format now produces an end date always, which now exceeds the valid date range.
This was essentially testing in the system time zone before. Now that evaluation requires a time zone, this required adjusting the local time to fit the default testing time zone of US Eastern.
It makes sense to be a nominal duration in this case, even if the spec does not say so specifically.
maknapp
force-pushed
the
zoned-occurrence
branch
from
e0d5fcd to
4e6b760
Compare
|
|
I think this is ready to merge. The failing checks are minor issues that will be addressed with discussed v6 changes. |
axunonb
left a comment
axunonb
left a comment
There was a problem hiding this comment.
Thanks, ready for next steps
|
Code coverage for version/6.0 is 0.50% lower than for main. |
* Use BCL HashCode * Fix date parsing in a test * Evaluate recurrence with nodatime * Evaluate calendar in a time zone always This evaluates events in the calendar for a specific time zone. Events with a time zone are evaluated in that event's time zone and then converted to the requested time zone. Floating events are evaluated in the request time zone. This is essentially the same behavior as before, except the results are actual point-in-time values. A key evaluation difference is that during backward daylight saving, the last time offset can be output instead of only the first. Before, an hourly repeating event would skip the second hour. Fixes #681, #875 * Change tests to fit zoned API The few tests still failing were left alone on purpose. With some tests, the test data needs to be changed because it does not make sense now that all occurrences are in a specific time zone. For FREEBUSY tests, that was mostly ignored because I am not sure how it should be used. * Remove invalid comparison operators The only valid comparison for two CalDateTime values is equality. The greater and less than operators cannot always return a valid result. This makes implementing IComparable also invalid. "20250815" != "20250815T000000" but these operators also did not say they were less or greater either. * Remove copying of CalDateTime CalDateTime is immutable, so there is no need to copy. * Add backward daylight saving tests * Change benchmarks to fit zoned API * Store CalDateTime values with NodaTime * Reduce evaluation allocation * Fix CalDateTime format test * Validate CalDateTime year CalDateTime was validating year using DateTime limits. NodaTime supports more than just positive years, so verify that the year is positive to match DateTime behavior. * Change out of range test This test had an effective end that exceeded the valid date range but only actually held a duration, so there was no exception. The new occurrence format now produces an end date always, which now exceeds the valid date range. * Change time zone test This was essentially testing in the system time zone before. Now that evaluation requires a time zone, this required adjusting the local time to fit the default testing time zone of US Eastern. * Removed warning It makes sense to be a nominal duration in this case, even if the spec does not say so specifically. * Validate EvaluationPeriod end * Fix freebusy validation message * Simplify CalDateTime LocalDateTime conversion * Change dateonly timeonly to methods * Remove unused DateUtil methods * Fix some sonar issues
3 participants
In response to #739 (exact comment), I started testing this out and got carried away with how much needed to change to make this work. This is only a draft proposal to change how evaluation works for version 6.
This changes the evaluation to use NodaTime and evaluates entirely in
ZonedDateTime. Events are evaluated in their own time zone, or the specified time zone if floating, and all occurrences are returned in the specified time zone. Two tests fail because the expected data should be different when evaluated in a time zone. A few other tests fail because of NodaTime allowed values and formatting changes.Notes:
CalDateTime.EvaluationPeriodhas all the information to produce the same DtStart, DtEnd/Duration, and time zone values as before, but I only did DtStart for passing tests.