Skip to content
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

Introduce Duration type to keep apart nominal vs exact durations. #680

Merged
merged 19 commits into from
Dec 28, 2024

Conversation

minichma
Copy link
Collaborator

@minichma minichma commented Dec 26, 2024

This PR addresses shortcomings of the lib regarding the different requirements of nominal vs exact duration as specified by RFC 5545. It introduces the new Duration type that replaces TimeSpan in most places. In contrast to the latter, Duration stores the individual duration parts (weeks, days, hours, minutes, seconds) individually. This is required because according to the RFC weeks and days are considered nominal values while hours, minutes, seconds are considered exact (see https://www.rfc-editor.org/rfc/rfc5545#section-3.3.6, https://www.rfc-editor.org/rfc/rfc5545#section-3.8.5.3).

Most relevant changes:

  • CalendarEvent.Duration et al and Period.Duration changed from TimeSpan to Duration
  • CalendarEvent.GetTimeSpanToAddToPeriodStartTime replaced by CalendarEvent.GetEffectiveDuration, which takes care of keeping apart nominal vs. exact durations. I.e. if the duration is specified via DTEND, then the returned duration only uses time-related fields, so it is considered exact. If specified via DURATION, the duration is returned as specified by the caller.
  • CalDateTime.Add(Duration) and .Subtract() handle the different parts of Duration conformant to the RFC.

Copy link

codecov bot commented Dec 26, 2024

Codecov Report

Attention: Patch coverage is 74.27184% with 53 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/DataTypes/Duration.cs 75% 3 Missing and 12 partials ⚠️
....Net/Serialization/DataTypes/DurationSerializer.cs 76% 7 Missing and 5 partials ⚠️
Ical.Net/DataTypes/Period.cs 59% 3 Missing and 6 partials ⚠️
Ical.Net/CalendarComponents/Alarm.cs 0% 5 Missing ⚠️
Ical.Net/CalendarComponents/CalendarEvent.cs 75% 1 Missing and 1 partial ⚠️
Ical.Net/DataTypes/CalDateTime.cs 93% 1 Missing and 1 partial ⚠️
Ical.Net/Evaluation/EventEvaluator.cs 67% 1 Missing and 1 partial ⚠️
Ical.Net/Evaluation/TodoEvaluator.cs 50% 1 Missing and 1 partial ⚠️
...al.Net/Serialization/DataTypes/PeriodSerializer.cs 60% 0 Missing and 2 partials ⚠️
Ical.Net/CalendarComponents/VTimeZone.cs 50% 1 Missing ⚠️
... and 1 more

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #680   +/-   ##
===================================
  Coverage    63%    63%           
===================================
  Files        99    100    +1     
  Lines      4550   4606   +56     
  Branches   1075   1107   +32     
===================================
+ Hits       2863   2909   +46     
+ Misses     1240   1239    -1     
- Partials    447    458   +11     
Files with missing lines Coverage Δ
Ical.Net/CalendarComponents/Todo.cs 69% <100%> (+5%) ⬆️
Ical.Net/DataTypes/CalendarDataType.cs 49% <100%> (ø)
Ical.Net/DataTypes/Trigger.cs 44% <100%> (ø)
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 71% <100%> (+<1%) ⬆️
Ical.Net/Serialization/DataTypeMapper.cs 78% <100%> (ø)
Ical.Net/Serialization/SerializerFactory.cs 79% <100%> (ø)
Ical.Net/Utility/DateUtil.cs 59% <100%> (+3%) ⬆️
Ical.Net/CalendarComponents/VTimeZone.cs 73% <50%> (ø)
...l.Net/Serialization/DataTypes/TriggerSerializer.cs 31% <67%> (ø)
Ical.Net/CalendarComponents/CalendarEvent.cs 73% <75%> (-1%) ⬇️
... and 8 more

... and 1 file with indirect coverage changes

@minichma minichma force-pushed the work/minichma/feature/duration branch from 04917a4 to 19e98d7 Compare December 26, 2024 22:30
@axunonb
Copy link
Collaborator

axunonb commented Dec 26, 2024

I'll be waiting until this PR gets merged before submitting a PR for EXDATE and RDATE fixes.

@minichma minichma force-pushed the work/minichma/feature/duration branch 2 times, most recently from 6462038 to 9922efc Compare December 27, 2024 15:42
@minichma
Copy link
Collaborator Author

I'll be waiting until this PR gets merged before submitting a PR for EXDATE and RDATE fixes.

@axunonb I think the PR should be ready for review, would be great if you'd find some time for it. It's quite some code that has changed, but all in all not too complex, once the basics regarding nominal vs. exact are understood. Probably it makes sense to review the code commit by commit. I tried to keep the commits reasonably separated, but probably not perfectly everywhere. There are certainly some smaller issues around nonexistent times remaining as outlined in #681.

@minichma minichma marked this pull request as ready for review December 27, 2024 15:48
@minichma minichma requested a review from axunonb December 27, 2024 15:48
@axunonb
Copy link
Collaborator

axunonb commented Dec 28, 2024

The plan for a next PR re Periodwas:

  • Change public Perdiod() to internal (should not be part of the public API, see Make parameterless CTORs only needed for ICopyable internal #676). ChangeActivator.CreateInstance(type, true) in CalendarDataType.Copy.
  • Timezone of StartTime and EndTime (if set) must be the same (as needed in the iCalendar context anyway)
    That's necessary for EXDATE and RDATE.

if (this.TzId is not null)
return SubtractExact(dt).ToDurationExact();

if (dt.HasTime != HasTime)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also tried throwing if IsFloating != dt.IsFloating, but then all kinds of tests fail. Obviously there are quite some cases where DTSTART has a tzid set but DTEND hasn't, which isn't allowed by the RFC though. We should double-check whether we still want to support this case (probably yes, if its used in the wild). If so, we should probably deal with the case in a different place, probably during deserialization, and be more strict here, because the current implementation is somewhat unclear regarding that case.

Copy link
Collaborator

@axunonb axunonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent.
See some minor remarks.
{Edit] Just forgot to mention the nullability warnings like Possible null reference argument for parameter 'dt' in 'Duration IDateTime.Subtract(IDateTime dt)' et al

@minichma minichma force-pushed the work/minichma/feature/duration branch from 9922efc to 8c9dba2 Compare December 28, 2024 16:26
…onSerializer` in preparation of using the type throughout the lib.
@minichma minichma force-pushed the work/minichma/feature/duration branch from 8c9dba2 to 07a55e6 Compare December 28, 2024 16:31
@minichma minichma force-pushed the work/minichma/feature/duration branch from bc558ea to 6566285 Compare December 28, 2024 17:03
@minichma minichma requested a review from axunonb December 28, 2024 17:06
@minichma
Copy link
Collaborator Author

@axunonb Thanks for the review! Tried to improve.

Copy link
Collaborator

@axunonb axunonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, thanks a lot

@axunonb axunonb merged commit 3e35d53 into main Dec 28, 2024
6 of 8 checks passed
@axunonb axunonb deleted the work/minichma/feature/duration branch December 28, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants