Reduce use of EvaluationPeriod#946
Conversation
DATE values should not have a time zone.
Removes hasTime parameter from CalDateTime constructors and requires DATE values to be created with a static method. This prevents the time zone parameter from being silently ignored when creating DATE values. Fixes #933
Time zone can be ignored for DATE values since they cannot have a time zone. This also verifies that end is not before start for DATE values.
This was only being used in tests.
Explicit names for ToDateTimeUtc and ToDateTimeUnspecified help clarify intention when using DateTime.
DateTime.Today is calculated from DateTime.Now but NodaTime also does the same calculation, so just use DateTime.Now to get the same result.
|
minichma
left a comment
minichma
left a comment
There was a problem hiding this comment.
I'm not the right person to review the whole PR, this is just a comment on using HashSet for EXDATEs. The original idea of using OrderedExclude was probably to be close to the implementation of EXRULEs, where it was more or less required. Once the set has been constructed, the computational cost per recurrence are also somewhat lower in case of OrderedExclude compared to a HashSet lookup, but that's neglectable. Now that it seems that support for EXRULEs has been dropped, there's no strong argument for OrderedExclude over a HashSet lookup. I assume the reason for removing it is to make the code easier to read and somewhat less complex to reason about, right? No concerns from my side.
Yes, sorry. That specific part was the reason I included you to review - probably should have just asked that directly @ you instead of including to review.
Yes, since it was the only remaining use of |
2 participants
Note that this is based off of #934, so really only 2 commits.
RecurrenceRuleEvaluatorwas just wrappingZonedDateTimein anEvaluationPeriodwith no end time, which meant anotherEvaluationPeriodneeded to be made to add an end time.RecurrenceRuleEvaluatornow yieldsZonedDateTimedirectly.RecurrenceUtil.GetOccurrences(...)no longer filters by start/end range because it is a duplicate filter toRecurringEvaluator.Evaluate().EXDATE date time exclusions were switched to
HashSetinstead of sorting and merging. There is no benefit to merging if the list needs to be sorted.To help reduce allocations and unnecessary work, RDATE merging and EXDATE filtering is skipped if empty.