Remove interface `IDateTime` and replace any use with `CalDateTime` #705

minichma · 2025-01-22T17:45:08Z

The changes in this PR are in based on the discussion in #705 (comment), where we discussed whether arithmetics and timezone-related operations on should be moved out of CalDateTime in favor of a more flexible approach where we could even configure different timezone databases in a future iteration. We decided against it for reasons of available resources, so arithmetics are kept as part of CalDateTime. Based on that the PR implements several improvements, most notably:

Remove the interface IDateTime in favor of using CalDateTime directly.
Simplify the type hierarchy of CalDateTime, i.e. stop it inheriting from EncodableDataType
Get rid of CalDateTime.AssociatedObject

codecov · 2025-01-22T17:47:55Z

Codecov Report

Attention: Patch coverage is 81.37255% with 19 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Ical.Net/CalendarComponents/Alarm.cs	0%	4 Missing ⚠️
....Net/Serialization/DataTypes/DateTimeSerializer.cs	85%	1 Missing and 2 partials ⚠️
Ical.Net/DataTypes/CalDateTime.cs	89%	1 Missing and 1 partial ⚠️
Ical.Net/DataTypes/CalendarDataType.cs	0%	2 Missing ⚠️
...l.Net/Serialization/DataTypes/TriggerSerializer.cs	0%	2 Missing ⚠️
Ical.Net/VTimeZoneInfo.cs	0%	2 Missing ⚠️
Ical.Net/DataTypes/AlarmOccurrence.cs	0%	1 Missing ⚠️
Ical.Net/DataTypes/Trigger.cs	50%	1 Missing ⚠️
Ical.Net/Evaluation/RecurrenceUtil.cs	0%	1 Missing ⚠️
...cal.Net/Serialization/DataTypeSerializerFactory.cs	0%	0 Missing and 1 partial ⚠️

❌ Your project status has failed because the head coverage (65%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage.

@@         Coverage Diff         @@
##           main   #705   +/-   ##
===================================
- Coverage    65%    65%   -0%     
===================================
  Files       103    103           
  Lines      4659   4642   -17     
  Branches   1131   1130    -1     
===================================
- Hits       3028   3004   -24     
- Misses     1197   1200    +3     
- Partials    434    438    +4

Files with missing lines	Coverage Δ
Ical.Net/Calendar.cs	`70% <ø> (ø)`
Ical.Net/CalendarCollection.cs	`57% <ø> (ø)`
Ical.Net/CalendarComponents/CalendarEvent.cs	`74% <100%> (ø)`
Ical.Net/CalendarComponents/FreeBusy.cs	`48% <100%> (ø)`
Ical.Net/CalendarComponents/RecurringComponent.cs	`65% <100%> (ø)`
Ical.Net/CalendarComponents/Todo.cs	`67% <100%> (ø)`
Ical.Net/CalendarComponents/UniqueComponent.cs	`56% <100%> (ø)`
Ical.Net/DataTypes/ExceptionDates.cs	`100% <ø> (ø)`
Ical.Net/DataTypes/Period.cs	`85% <100%> (ø)`
Ical.Net/DataTypes/PeriodList.cs	`88% <ø> (ø)`
... and 26 more

... and 5 files with indirect coverage changes

axunonb · 2025-01-23T22:03:12Z

Ical.Net/DataTypes/CalDateTime.cs

@@ -23,7 +23,7 @@ namespace Ical.Net.DataTypes;
 /// This is because RFC 5545, Section 3.3.5, does not allow for fractional seconds.
 /// </remarks>
 /// </summary>
-public sealed class CalDateTime : EncodableDataType, IComparable<CalDateTime>, IFormattable
+public sealed class CalDateTime : IComparable<CalDateTime>, IFormattable


... or even struct? (not in this PR probably)

Yes, struct would certainly be good. But we'd have to deal with all the nullability stuff first, which is not perfectly straight forward. Next step could then be to do all that also for Period, PeriodList and maybe others.

axunonb · 2025-01-24T23:47:15Z

Ical.Net/Utility/DateUtil.cs

@@ -15,7 +15,7 @@ namespace Ical.Net.Utility;

 internal static class DateUtil
 {
-    public static DateTime GetSimpleDateTimeData(IDateTime dt)
+    public static DateTime GetSimpleDateTimeData(CalDateTime dt)


Should we remove this method? Using dt.Value is shorter and more clear

…re).

sonarqubecloud · 2025-01-27T20:37:10Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

axunonb · 2025-01-31T15:53:34Z

This is PR is still in progress, right? Just asking because I'm assigned to review.

minichma · 2025-01-31T23:36:22Z

Yes, it is still work in progress. I'm not quiet convinced, this is the right approach. Will try to comment more this weekend.

minichma · 2025-02-02T13:13:59Z

@axunonb I apologize for the delay, don't find sufficient time recently. I continued the discussion in #606 (reply in thread), as this is not really specific to this PR. Please let me know what you think!

minichma · 2025-02-02T19:20:11Z

Based on the discussion in #705 (comment) we decided to proceed, so the PR should be ready for review.

axunonb · 2025-02-02T22:42:15Z

public CalDateTime() { } should be internal or even private - nothing for the public API

axunonb · 2025-02-02T22:44:32Z

Ical.Net.Tests/DeserializationTests.cs

@@ -107,7 +107,7 @@ public void Bug2033495()
    }

    /// <summary>
-    /// Tests bug #2938007 - involving the HasTime property in IDateTime values.
+    /// Tests bug #2938007 - involving the HasTime property in CalDateTime values.


Summary could be removed?

axunonb · 2025-02-05T08:36:31Z

Ical.Net/CalendarComponents/CalendarEvent.cs

@@ -54,9 +54,9 @@ private void SetProperty<T>(string group, T value)
    /// will be extrapolated.
    /// </note>
    /// </summary>
-    public virtual IDateTime? DtEnd
+    public virtual CalDateTime? DtEnd


Should we now remove the aliases End => DtEnd and Start => DtStart and just use the more intuitive End and Start everywhere?
(The underlying Properties could stay unchanged)

axunonb · 2025-02-05T08:40:08Z

Ical.Net/CalendarComponents/IUniqueComponent.cs

@@ -15,7 +15,7 @@ public interface IUniqueComponent : ICalendarComponent

    IList<Attendee> Attendees { get; set; }
    IList<string> Comments { get; set; }
-    IDateTime DtStamp { get; set; }
+    CalDateTime DtStamp { get; set; }


or just Stamp?

axunonb · 2025-02-05T08:44:06Z

Ical.Net/DataTypes/CalDateTime.cs

-    /// <inheritdoc cref="TzId"/>
+    /// <summary>
+    /// Gets the timezone name this time is in, if it references a timezone.
+    /// </summary>
    /// <remarks>This is an alias for <see cref="TzId"/></remarks>
    public string? TimeZoneName => TzId;


Remove the alias?

axunonb · 2025-02-05T09:00:38Z

Ical.Net/Utility/DateUtil.cs

@@ -16,9 +16,6 @@ namespace Ical.Net.Utility;

 internal static class DateUtil
 {
-    public static DateTime GetSimpleDateTimeData(IDateTime dt)


axunonb · 2025-02-05T09:02:18Z

Ical.Net/VTimeZoneInfo.cs

@@ -133,15 +133,15 @@ public virtual IList<string> TimeZoneNames
        set => Properties.Set("TZNAME", value);
    }

-    public virtual IDateTime DtStart
+    public virtual CalDateTime DtStart


There are many of these aliases, so maybe rather address remove the the Dt... properties in a separate PR

axunonb

Very nice. See few minor comments.

minichma mentioned this pull request Jan 22, 2025

Replace public methods / properties using DateTime with CalDateTime #703

Open

minichma force-pushed the work/minichma/feature/remove_idatetime branch from a85dd77 to a09dcf2 Compare January 23, 2025 19:18

axunonb reviewed Jan 23, 2025

View reviewed changes

axunonb reviewed Jan 24, 2025

View reviewed changes

minichma added 6 commits January 27, 2025 21:31

Remove interface IDateTime. Find/replace IDateTime -> CalDateTime.

a96cdd6

Adjust CalDateTime inheritance, copy method doc comments from IDateTime.

bff7a2e

WIP: Stop using and populating CalDateTime.AssociatedObject

ac1f3e5

CalDateTime: Simplify type hierarchy.

9bd3692

CalDateTime.ToTimeZone param is nullable.

b3423ba

Remove DateUtil.GetSimpleDateTime(), which isn't too helpful (anymo…

0d1cd25

…re).

minichma force-pushed the work/minichma/feature/remove_idatetime branch from 1679c9f to 0d1cd25 Compare January 27, 2025 20:36

minichma marked this pull request as ready for review February 2, 2025 19:20

axunonb self-requested a review February 2, 2025 22:30

axunonb reviewed Feb 2, 2025

View reviewed changes

axunonb reviewed Feb 5, 2025

View reviewed changes

axunonb approved these changes Feb 5, 2025

View reviewed changes

axunonb merged commit f6f502f into main Feb 10, 2025
7 of 8 checks passed

axunonb deleted the work/minichma/feature/remove_idatetime branch February 10, 2025 09:48

minichma mentioned this pull request Feb 16, 2025

TBD: Make parsing of CalDateTime strings more restrictive #734

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove interface `IDateTime` and replace any use with `CalDateTime` #705

Remove interface `IDateTime` and replace any use with `CalDateTime` #705

minichma commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

axunonb Jan 23, 2025 •

edited

Loading

minichma Jan 24, 2025

axunonb Jan 24, 2025

sonarqubecloud bot commented Jan 27, 2025

axunonb commented Jan 31, 2025

minichma commented Jan 31, 2025

minichma commented Feb 2, 2025

minichma commented Feb 2, 2025

axunonb commented Feb 2, 2025

axunonb Feb 2, 2025

axunonb Feb 5, 2025

axunonb Feb 5, 2025

axunonb Feb 5, 2025

axunonb Feb 5, 2025

axunonb Feb 5, 2025

axunonb left a comment

Remove interface IDateTime and replace any use with CalDateTime #705

Remove interface IDateTime and replace any use with CalDateTime #705

Conversation

minichma commented Jan 22, 2025 • edited Loading

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

axunonb Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 27, 2025

Quality Gate passed

axunonb commented Jan 31, 2025

minichma commented Jan 31, 2025

minichma commented Feb 2, 2025

minichma commented Feb 2, 2025

axunonb commented Feb 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axunonb left a comment

Choose a reason for hiding this comment

Remove interface `IDateTime` and replace any use with `CalDateTime` #705

Remove interface `IDateTime` and replace any use with `CalDateTime` #705

minichma commented Jan 22, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

axunonb Jan 23, 2025 •

edited

Loading