-
Notifications
You must be signed in to change notification settings - Fork 233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add abstraction layer to create EXDATE
and RDATE
properties
#684
Conversation
9d19aa8
to
f09b049
Compare
@minichma The code changes became more than expected, because it was necessary to revisit |
I'm pretty sure it is. I see the challenges that come with introducing separate types for lists of Periods and IDateTimes - redundant code, challenges with type safety, inheritance. At the same time I remember the challenges I had when starting with ical.net and not knowing much about the details of RFC 5545. The ExDate property being a list of Periods was not exactly easy to understand at that time. So yes, doc comments will certainly be helpful in this respect. We can still continue thinking about alternatives in the future (maybe not v5 though once it has been released). |
Oops, just noticed that |
76a047e
to
9eef103
Compare
Creating |
@axunonb Wish you a great 2025! Hope I will find some time for a review this afternoon when I'll be on a train or otherwise tomorrow. |
9eef103
to
f762d8f
Compare
@minichma Happy New Year and thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a challenging one! Left a few comments but they might not be fully consistent. Just take what you think makes sense. Unfortunately I don't have the time right now to go into all the detail that would be required.
f762d8f
to
b54ca8c
Compare
@minichma Looks like the failing tests for .Net Framework go back to GitHub upgrading to |
0842c10
to
e190317
Compare
Improve reliability and usability for `Period` and `PeriodList` to become less error-prone by enforcing timezones being used consistently Period - Make parameterless CTOR `internal` to ensure proper initialization by users - A period can be defined 1. by a start time and an end time, 2. by a start time and a duration, 3. by a start time only, with the duration unspecified. This is for EXDATE and RDATE date-only and date/time. - For cosistency, either the `EndTime` or the `Duration` can be set at a time. The last one set with a value not `null` will prevail, while the other will become `null`. - Timezones of `StartTime`and (optional) `EndTime` must be the same - `CompareTo` uses `AsUtc` for comparing the `StartTime` - Remove returning a "magic" duration of 1 day, if `EndTime` is null and `StartTime` is date-only. This broke `EXDATE` and `RDATE` of an event, when the only a start time exists. - Added `EffectiveEndTime` and `EffectiveDuration` properties to provide calculated values based on the set values. - Update the `EndTime` and `Duration` properties to directly return the set values. - Change the access modifiers of `GetEffectiveDuration()` and `GetEffectiveEndTime` methods from internal to private. PeriodList - `TzId`: `public` setter changed to `private` - `EnsureConsistentTimezones`: The first period determines the timezone of the `PeriodList` and all other `Period`s added must have the same timezone - Add `SetService(new PeriodListEvaluator(this))` for `StringReader` CTOR overload - Add `static PeriodList FromStringReader(StringReader)` - Add `static PeriodList FromDateTime(IDateTime)` - Add `PeriodList AddPeriod(Period)` for chaining - Add `PeriodList Add(IDateTime)` for chaining - nullable enable EventEvaluator: - `EventEvaluator.WithEndTime(Period)` only sets the `EndTime`, as `Period.EffectiveDuration` returns the duration. TodoEvaluator: - Remove method `PeriodWithDuration(Period)` as it became redudant with the refactored `Period` class. DataTypeSerializer and other serializers, CalendarObjectBase: - `Activator.CreateInstance(TargetType, true)` allows for not `public` CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like with `Period`). PropertySerializer: - TBD: Never set the UTC timezone ID (use appending 'Z') - Resolves ical-org#590 - Resolves ical-org#591 - Resolves ical-org#614 - Resolves ical-org#676
PeriodKind - Add new enum Period: - Add internal `property string TzId` - Add internal method `PeriodKind GetPeriodKind()` - Ensure timeone consistence for setters of StartTime / EndTime PeriodList: - Add internal `property string TzId` - Add internal property ``PeriodKind PeriodListKind`m - Ensure added `Period`s have the same `TzId`and `PeriodKind` as the first one added PeriodListSerialilzer - Serializes TZID - Serializes value type PERIOD
- Introduced `ExceptionDateCollection`, `RecurrencePeriodCollection` and PeriodCollectionBase` classes. - Updated serializers to handle the modified `PeriodList` implementation. These classes are an abstraction layer for creating `List<PeriodList>` objects. `Period`s and `CalDateTime`s are accepted without having to care for RFC 5545 rules regarding serialization: `ToRecurrenceDates` and `ToRecurrenceDates` methods aggregate and convert the exception or recurrence `CalDateTime` and `Period` objects into a list of `PeriodList` objects. Periods are grouped by their timezone IDs and period kinds in a way, that each `PeriodList` contains only distinct periods ready to serialize.
- Adjust code in ' EventEvaluator` and `PeriodSerializer` - Method `GetPeriodKind()` is replace with internal property `PeriodKind` - Add internal `Period.Create` to centralize logic if end time and duration exist - Modified unit test
Remove solution for RDATE/ESDATE introduced earlier in this PE The commit removes several test and source files related to period and exception date collections in calendar events. Specifically, the following files have been completely removed: - `PeriodCollectionBase.cs` - `ExceptionDateCollection.cs` - `RecurrencePeriodCollection.cs` - `PeriodCollectionTests.cs` - `RecurrenceWithExDateTests.cs` - `RecurrenceWithRDateTests.cs`
66a1611
to
fca144a
Compare
- Add internal properties `PeriodKind` and `TzId` to `PeriodList`. - Modify `Period` indexer and `Insert` to ensure consistency for timezone and `PeriodKind` - Prepare for `PeriodList` to become `internal` - Update `PeriodListSerializer` and `PeriodSerializer` classes for `Period` serialization - Introduce `ExceptionDates`, `RecurrenceDates`, and `PeriodListWrapperBase` to become wrappers around these properties of `RecurringComponent` - Add unit tests in `PeriodListWrapperTests` for `ExceptionDates` and `RecurrenceDates`. Note: The wrappers are not yet implemented for `IRecurringComponent`s.
fca144a
to
3800f1c
Compare
…nceDates` `IRecurrable` - Change the type of `ExceptionDates` from `IList<PeriodList>` to `ExceptionDates`. - Change the type of `RecurrenceDates` from `IList<PeriodList>` to `RecurrenceDates`. `RecurringComponent`, `VTimeZoneInfo` - Rename `IList<PeriodList> ExceptionDates` to `ExceptionDatesPeriodLists`. This serves as the backing storage for the new `ExceptionDates` class/property. - Rename `IList<PeriodList> RecurrenceDates` to `RecurrenceDatesPeriodLists`. This serves as the backing storage for the new `RecurrenceDates` class/property. - Add the creation of instances of `ExceptionDates` and `RecurrenceDates` to the `Initialize()` method. `PeriodList` and `PeriodListEvaluator` - Change the access modifier to `internal`. `RecurringEvaluator` and `TodoEvaluator` - Implement the new `ExceptionDates` and `RecurrenceDates` classes. `VTimeZone` - Enhanced serialization: - Before: Each `RDATE` date/time was serialized as a separate `RDATE` propery - Now: `RDATE` date/times which can be clustered go into one single `RDATE` property Unit tests - Refactor tests, using `ExceptionDates` and `RecurrenceDates` instead of `IList<PeriodList>` - Remove the duplicate `SimpleDeserializationTests.RecurrenceDates1()` method, which exists in `DeserializationTests.RecurrenceDates1()`. Todo: - Replace the use of `PeriodList.GetGroupedPeriods` in `CalendarEvent.Equals()`.
9f7d6ef
to
3817d85
Compare
@minichma Also a long story has end... |
Using this internal method stops recreating `Period`s from `IDateTime` Update comments in PropertySerializer
Period
and PeriodList
to work with EXDATE
and RDATE
EXDATE
and RDATE
properties
@axunonb Will be happy to have a look. Could still take a few days though because of family obligations and a busy week. |
- No duplicate `Period` instances can be added or inserted into the `PeriodList`. - `RecurrenceDates.GetAllDates()`, `PeriodListWrapperBase.GetAllDates()` and `PeriodListWrapperBase.GetAllPeriodsByKind` return a flattened and ordered list of distinct objects - Update `CalendarEvent.Equals` and `CalendarEvent.GetHashCode` using `ExceptionDates` and `RecurrenceDates` instead of `PeriodList.GetGroupedPeriods` - Remove `PeriodList.GetGroupedPeriods`
14b0187
to
dc4f248
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axunonb Had a rather brief look to the most relevant portions and left a few comments again, maybe some duplicated. Please see, what you can use.
Ical.Net/DataTypes/Period.cs
Outdated
|
||
StartTime = start ?? throw new ArgumentNullException(nameof(start)); | ||
EndTime = end; | ||
_startTime = start ?? throw new ArgumentNullException(nameof(start), "Start time cannot be null."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd have seen a NPE if start was null, at least if end was not nulll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Ical.Net/DataTypes/Period.cs
Outdated
{ | ||
null when _duration is null => null, | ||
{ } endTime => endTime, | ||
_ => EffectiveDuration is not null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid evaluating EffectiveDuration twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Ical.Net/DataTypes/Period.cs
Outdated
// End time is exclusive | ||
return endTime == null || endTime.GreaterThan(dt); | ||
return endTime?.GreaterThan(dt) ?? false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a functional change, i.e. if EffectiveEndTime is null. Before dt was considered contained and now it isn't. I assume the new version is better, but just to be sure, this was intentional. Could also throw in such a case, because 'Contains' doesn't make much sense in such a case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must be return endTime?.GreaterThan(dt) != false;
@@ -155,14 +226,14 @@ internal IDateTime GetEffectiveEndTime() | |||
public virtual bool Contains(IDateTime? dt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of convenience methods could be changed to extension methods (in some future PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
public int CompareTo(Period? other) | ||
{ | ||
if (other == null) | ||
{ | ||
return 1; | ||
} | ||
|
||
if (StartTime.Equals(other.StartTime)) | ||
if (StartTime.AsUtc.Equals(other.StartTime.AsUtc)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do a UTC comparison here? I.e. equality is quite a topic in CalDateTime, so why would we use a different comparison here? Do we need the comparison at all? It seems to implement a somewhat special case. I don't think there's a natural order if the duration isn't considered. Maybe remove the comparison altogether and let the caller choose how to compare?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, you remember the discussion about CalDateTime
equality.
I opened a discussion for that topic.
We can't use Exact Equivalence (same IsFloating
and same Value
and same HasTime
and same TzId
) here, because the timezone of 2 Periods may be different. and.AsUtc
is a must.
Isn't the CompareTo
method specific to each component, that the all callers should "just use".
/// <see cref="PeriodKind.Period"/>, <see cref="PeriodKind.DateOnly"/> and <see cref="PeriodKind.DateTime"/>. | ||
/// </summary> | ||
internal IEnumerable<Period> GetAllPeriodsByKind(params PeriodKind[] periodKinds) | ||
=> ListOfPeriodList.SelectMany(pl => pl.Where(p => periodKinds.Contains(p.PeriodKind))).OrderedDistinct(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use OrderedDistinct
only if the list it is applied on is ordered. But this would only be the case if the individual lists would be ordered and they would be merged using OrderedMerge
. But do we need to apply a Distinct at all? If the values are duplicated, why not return them here? The method name suggests that 'all' are returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - RTD., thanks. When attempting to add a duplicate, it just gets discarded. For consistency, values from deserialization should also be unique.
/// </summary> | ||
public bool Contains(Period period) | ||
{ | ||
var periodList = GetPeriodList(period); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above - GetPriodList doesn't necessarily return the right list.
/// <inheritdoc cref="PeriodList.Remove"/> | ||
public bool Remove(Period period) | ||
{ | ||
var periodList = GetPeriodList(period); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above, possibly not the right list
// specifies a "DTSTART" property with a DATE value type but no | ||
// "DTEND" nor "DURATION" property, the event’s duration is taken to | ||
// be one day. | ||
// This is taken care of in the PeriodSerializer class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we should remove this as the Period only comes into play during evaluation, but the CalendarEvent's duration could be queried independently. Either we remove the method altogether or we return a precise value. We could consolidate the calculation though. I would also not consider this a 'magic number' though. It's just the value that's specified in the RFC.
We have the same kind of logic in multiple places now, so we could consolidate it in a utility class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid point - reverted
var otherRDates = PeriodList.GetGroupedPeriods(other.RecurrenceDates); | ||
if (rDates.Keys.Count != otherRDates.Keys.Count || !rDates.Keys.OrderBy(k => k).SequenceEqual(otherRDates.Keys.OrderBy(k => k))) | ||
// exDates and otherExDates are filled with a sorted list of distinct periods | ||
var exDates = ExceptionDates.GetAllPeriodsByKind(PeriodKind.Period, PeriodKind.DateOnly, PeriodKind.DateTime).ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need an implementation of Equals at all? Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, we don't for CalendarEvent
. Should go into another PR.
Quality Gate passedIssues Measures |
@minichma Thanks for having another look at the PR. As always: very helpful. See the comments for details. |
@axunonb Your are welcome :) |
* Improve line folding and serialization performance - Update line folding to use StringBuilder and ArrayPool<char> for better performance and memory efficiency. - Ensure compliance with RFC 5545 by handling multi-byte characters correctly. - Add System.Buffers package for netstandard2.0 target framework. Resolves #693 * Remove code redundant after #684 * Replace usage of ArrayPool, remove dependency System.Buffers
Add abstraction layer to create
EXDATE
andRDATE
propertiesExceptionDate
andRecurrenceDates
classes as wrappers forIList<PeriodList>
properties.Period
s andCalDateTime
s to them can be done without having to care for RFC 5545 rules regarding serialization:. This happens behind the scenes.IRecurrable
ExceptionDates
fromIList<PeriodList>
toExceptionDates
.RecurrenceDates
fromIList<PeriodList>
toRecurrenceDates
.RecurringComponent (inhereted by CalendarEvent and other classes), VTimeZoneInfo
IList<PeriodList> ExceptionDates
toExceptionDatesPeriodLists
. This serves as the backing storage for the newExceptionDates
class/property.IList<PeriodList> RecurrenceDates
toRecurrenceDatesPeriodLists
. This serves as the backing storage for the newRecurrenceDates
class/property.ExceptionDates
andRecurrenceDates
to theInitialize()
method.RecurringEvaluator and TodoEvaluator
ExceptionDates
andRecurrenceDates
classes.VTimeZone
RDATE
date/time was serialized as a separateRDATE
properyRDATE
date/times which can be clustered go into one singleRDATE
propertyPeriod
internal
to ensure proper initialization by usersPeriodKind
to identify the period kind asUndefined
,DateTime
,DateOnly
orPeriod
and propertyPeriodKind
StartTime
,EndTime
andDuration
are immutableStartTime
and (optional)EndTime
must be the sameCompareTo
usesAsUtc
for comparing theStartTime
EndTime
is null andStartTime
is date-only. This brokeEXDATE
andRDATE
of an event, when only a start time exists. Full day is handled byPeriodSerializer
.EffectiveEndTime
andEffectiveDuration
properties to provide calculated values based on the set values.PeriodList
internal
.internal PeriodKind PeriodKind
internal string? TzId
, returning the TzId of the first periodEnsureConsistentTimezoneAndPeriodKind
: The first period determines theTzId
andPeriodKind
of thePeriodList
and all otherPeriod
s added or inserted must have these properties as well.SetService(new PeriodListEvaluator(this))
forStringReader
CTOR overloadAdd(IDateTime)
public
methodGetGroupedPeriods
PeriodListEvaluator
internal
EventEvaluator:
RDATE
durations correctlyTodoEvaluator:
ExceptionDate
andRecurrenceDates
classesPeriodWithDuration(Period)
as it became redudant with the refactoredPeriod
class.RecurringEvaluator:
ExceptionDate
andRecurrenceDates
classesDataTypeSerializer and other serializers, CalendarObjectBase:
Activator.CreateInstance(TargetType, true)
allows for notpublic
CTORs, so that parameterless CTORs can be excluded from the public API, if proper initialization can't be assured (like withPeriod
).PeriodListSerializer, PeriodSerializer, DateTimeSerializer, PropertySerializer
PeriodList
implementation.RDATE
andEXDATE
serialize following RFC 5545 sections 3.8.5.2 and 3.8.5.1Resolves #590
Resolves #591
Resolves #614
Resolves #676