-
Notifications
You must be signed in to change notification settings - Fork 247
feat: RecurringComponent supports RECURRENCE-ID with optional RANGE parameter
#870
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
Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #870 +/- ##
=======================================
+ Coverage 67.9% 68.1% +0.2%
=======================================
Files 112 115 +3
Lines 4297 4353 +56
Branches 973 1003 +30
=======================================
+ Hits 2919 2966 +47
+ Misses 1035 1028 -7
- Partials 343 359 +16
🚀 New features to boost your workflow:
|
0d48777 to
3758134
Compare
|
@minichma How about going the first step in terms of |
1c3d890 to
8e3d263
Compare
…NGE` parameter * Marked `CalDateTime? RecurrenceId` as obsolete in favor of `RecurrenceId? RecurrenceInstance`, but both properties can still be used in parallel, when the `RANGE` parameter is not required * Added new data type `RecurrenceId` * Added new `RecurrenceIdSerializer`
8e3d263 to
2c41cfe
Compare
minichma
left a comment
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.
LGTM! Based on that change the actual filtering should be quite straight forward. Let me know if you'd like me to assist there.
Ical.Net/DataTypes/RecurrenceId.cs
Outdated
| /// This class is used to uniquely identify a particular occurrence within a recurring series. The | ||
| /// identifier consists of a date and a recurrence range, which together specify the instance. | ||
| /// </remarks> | ||
| public class RecurrenceId : ICalendarParameterCollectionContainer |
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.
Should probably have Equals/GetHashCode
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.
Now implements IComparable<RecurrenceIdentifier>
| var dtSerializer = factory.Build(typeof(CalDateTime), SerializationContext) as DateTimeSerializer; | ||
|
|
||
| recurrenceId.Parameters.AddRange(dtSerializer!.GetParameters(recurrenceId.StartTime)); | ||
| if (recurrenceId.Range == RecurrenceRange.ThisAndFuture) |
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.
fail on unknown ones?
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.
log warning
| s = new CalendarSerializer(ctx); | ||
| return new CalendarSerializer(ctx); | ||
| } | ||
| else if (typeof(ICalendarComponent).IsAssignableFrom(objectType)) |
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 would be a perfect fit for pattern matching nowadays.
Ical.Net/VTimeZoneInfo.cs
Outdated
| /// With <see cref="RecurrenceRange.ThisAndFuture"/>, the instance applies to the specified | ||
| /// <see cref="RecurrenceId.StartTime"/> and all future occurrences. | ||
| /// </summary> | ||
| public virtual RecurrenceId? RecurrenceInstance |
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.
Consider naming?
|
Refactoring from review Renamed the
Renamed the
Added class
Miscellaneous
Note: Did not investigate possible code duplication in
Oh yes, that would be more than welcome. |
Renamed the `RecurrenceId` class to `RecurrenceIdentifier` * Removed `ICalendarParameterCollectionContainer` because custom parameters should not be allowed here * Added `IComparable<RecurrenceIdentifier>` Renamed the `RecurrenceIdSerializer` class to `RecurrenceIdentifierSerializer` * Added `IParameterProvider` Added class `ParameterProviderHelper` that contains methods * `GetCalDateTimeParameters` * `GetRecurrenceIdentifierParameters` Miscellaneous * Update `DateTimeSerializer.GetParameters` to call `ParameterProviderHelper.GetCalDateTimeParameters` * Update `IParameterProvider.GetParameters` to accept a nullable object * Update `SerializerFactory.Build` to use pattern matching
12515c1 to
e76de2f
Compare
|
Having a closer look at the RFC, my assumption might be mistaken. 😆 Anyhow, will give it a try. |
|
Hm, my naive assumption was as follows, but seems to be wrong:
My assumption was that the new component may or may not have an RRULE or RDATE. If it hasn't, it produces just a single occurrence. But looking at the RFC
this sounds more like the new VEVENT (the one with the RECURRENCE-ID) doesn't have its own RRULE/RDATE/EXDATE properties but inherits those of the original one, only the offset is considered, right? If so, several questions arise:
Had a look what Google Calendar and Outlook would export in case of modified time series.
Any comments are welcome. Short, precise take based on RFC 5545 and vendor docs: Mental model (what to implement)
Answers to your questions
Implementation sketch (deterministic)
References: RFC 5545 §3.8.4.4 ( If you want, I can turn this into a small testable expansion/override function (e.g., Python with |
|
As the changes in this PR are an improvement also without having the full evaluation implemented, it might be a good idea to complete this one and create a new one for the rest. This would avoid delaying this one and would remove time pressure. What do you think? |
|
First of all thanks for the reviews, which are always inspiring!
Yes, that's what I had in mind as well when mentioning "implementation for recurrence not included".
Kind of 😉, I'm afraid. Google doesn't support it, and I also understood that we should follow the Microsoft way: Never created a sub issue before, but this topic could a good trial for creating one to #455. I'll create it, and there we could discuss further details. |


VTimeZoneInfoandRecurringComponent:CalDateTime? RecurrenceIdas obsolete in favor ofRecurrenceId? RecurrenceInstance, but both properties can still be used in parallel, when theRANGEparameter is not requiredRecurrenceIdentifierRecurrenceIdentifierSerializerSupports #455 - implementation for recurrence not included