Enhance CalDateTime and DateTimeSerializer #638

axunonb · 2024-11-09T15:46:55Z

Breaking Changes

Removef the redundant Calendar argument to constructor overload of CalDateTime
CalDateTilme only uses DateTilmeKind.Utc and DateTimeKind.Unspecified. DateTimeKind.Local has been dropped.
Setting null for the timezone will apply the systems local time withAsDateTimeOffset`.
The given timezone always takes precedence over any DateTimeKind of a DateTime value. This is also the case, if a UTC date and a timezone (not null) is used.
CalDateTime.TzId: Will not read or write CalDateTime.Parameters. The functionality has been moved to `DateTimeSerializer.Deserialize(TextReader)
DateTimeSerializer.Deserialize(TextReader) will now throw for unrepresentable date/time values.
CalDateTime.TzId can be null or a valid timezone ID. Whitespace is not allowed.

CalDateTime

ImplementDateOnly and TimeOnly in order to return correct values for CalDateTime.HasTime and CalDateTime.HasDate, even if hour, minutes and seconds are zero.
Updating Value or TzId will re-initialize the instance in the same way as with CTOR overloads.
Refactored ToString overloads to convert the date and time to a string representation. Thanks to @nzbart for his initial PR.
marked some properties as obsolete.
Updated related tests and methods to accommodate these changes.
Enable NRT
Complete xmldoc
Increase code coverage to 94%

DateTimeSerializer

Added validation in the SerializeToString method to ensure the TZID parameter is correctly handled based on whether the date-time is in UTC.
Improved the Deserialize method to handle both full date-time and date-only strings, initializing the DateOnly and TimeOnly parts accordingly.
Enable NRT
Complete xmldoc
Increase code coverage to 94%

Other

Enable NRT for IDateTime
Added new package reference Portable.System.DateTimeOnly for netstandard2.0 and netstandard2.1 for compatibility with net6.0 and later.

Resolves #630
Resolves #633
Resolves #635
Resolves #636
Resolves #637
Resolves #197

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

Ical.Net/DataTypes/CalDateTime.cs

axunonb · 2024-11-11T22:48:32Z

Ical.Net/DataTypes/CalDateTime.cs

                {
+                    Initialize(_value, _timeOnly.HasValue, value, Calendar);


Changing the TzId now initializes with immediate effect

Ical.Net/DataTypes/Period.cs

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-11-12T08:30:26Z

Ical.Net/DataTypes/CalDateTime.cs

 /// </remarks>
 /// </summary>
 public sealed class CalDateTime : EncodableDataType, IDateTime
 {
+    // The date and time parts that were used to initialize the instance
+    // or by the Value setter.
+    private DateTime _value;


I think, we shouldn't maintain both, the DateTime value and Date-/TimeOnly. I suggest to remove _value altogether.

Without the backing value we lose the HasDate and HasTime "toggle". The Date-/TimOnly fields are just the replacement for the backing _hasDate and _hasTime booleans. This implementation is mainly because HasDate and HasTime require a setter from the IDateTime interface.

I thought it would be an option to calculate the DateTime value from the other two just in time, when needed.

I think I get your point: HasDate and HasTime wouldn't have backing fields, they would just become pure boolean getters/setters. Then _value can disappear. Yep, that's better.

Will be updated as proposed.

minichma · 2024-11-12T08:50:16Z

Ical.Net/DataTypes/CalDateTime.cs

-    private void Initialize(DateTime value, string tzId, Calendar cal)
-    {
-        if (!string.IsNullOrWhiteSpace(tzId) && !tzId.Equals("UTC", StringComparison.OrdinalIgnoreCase))
+        if ((tzId != null && !string.IsNullOrWhiteSpace(tzId) && !tzId.Equals("UTC", StringComparison.OrdinalIgnoreCase))


I know, it's legacy (so nothing to necessarily deal with in this PR), but I think that we shouldn't have special handling for white space, or empty strings. Either the value is set (!= null) or not (== null). Input sanitization is not something this class should be responsible for.

Will be updated as proposed.

minichma · 2024-11-12T09:02:28Z

Ical.Net/DataTypes/CalDateTime.cs

-            TzId = tzId;
+            _tzId = tzId;
+
+            initialValue = DateTime.SpecifyKind(dateTime, DateTimeKind.Local);


Also legacy, but IMHO important: I think we should avoid DateTimeKind.Local wherever we can, because dealing with the system's local time really is a special case. If no tzid is specified, this would usually mean that the instance represents floating time (which btw cannot be converted to a different time zone). Floating time should rather be represented by DateTimeKind.Unspecified than .Local.

Will be updated as proposed.

minichma · 2024-11-12T09:24:37Z

@axunonb I only had a very brief look at the new CalDateTime class. While I think that this PR brings some improvements, I feel that the whole design of the CalDateTime class deserves some more attention and improvements. Just a few thoughts:

I feel that NodaTime's TimeOnly/DateOnly or maybe LocalDateTime would be better suitable for representing the inner value than System.DateTime, because its more clearly structured and doesn't have the ambiguity that comes from the .Kind field. Anyhow I recommend to have just either of them, not both.
I think that CalDateTime in general should be immutable (maybe implemented as value type, though that's probably not that important).
I'm not sure, whether we need the IDateTime interface, do we? CalDateTime seems to be the only implementation and the additional interface seems to rather introduce additional complexity than reducing it.
Not sure, what the AssociatedObj property is used for but I feel that it shouldn't be required for CalDateTime. I understand that it's part of the overall Calendar model infrastructure, but maybe that's a core problem of the whole CalDateTime class, that it mixes up the representation of a value in the calendar model with the operations to be executed on top of it. In case of recurrence evaluation there's a rather clear separation between the model (e.g. RecurrencePattern) and the evaluation (RecurrencePatternEvaluator), but in the case of CalDateTime this doesn't exist, which can easily cause confusion. Maybe we should put some thoughts into how this separation could be improved. But I don't have a clear picture of how this would ideally look like either. Anyhow, as long as its used for representing the calendar model, the type should do that with as little ambiguity as possible. Any calculations on top should be performed just in time while the data of the original model should stay unchanged and represent the original value.

jm2c

axunonb · 2024-11-13T08:12:33Z

Todo in this PR:

Use of `NodaTime` for Date and Time Representation

Suggestion: Use ~~NodaTime's LocalDateTime,~~ DateOnly, or TimeOnly instead of System.DateTime.

Reasoning: NodaTime provides a more precise and unambiguous representation of date and time values. System.DateTime can be ambiguous due to the Kind property, which can lead to confusion about whether the time is local, UTC, or unspecified.

Action: Consider refactoring CalDateTime to use DateOnly andTimeOnly types. This will make the class more robust and less prone to errors related to time zone handling.

Timing: In #638

Necessity of `IDateTime` Interface

Suggestion: Evaluate the necessity of the IDateTime interface.

Reasoning: If CalDateTime is the only implementation of IDateTime in ical.net, the interface might be unnecessary and could introduce additional complexity without providing significant benefits.

Action: Review the usage of IDateTime. If it is not providing clear benefits, consider removing it and using CalDateTime directly.

Timing: Together with another breaking change. ~~In #638 mark HasTime and HasDate setters as obsolete.~~

AssociatedObject Property

Suggestion: Re-evaluate the need for the AssociatedObject property in CalDateTime.

Reasoning: The AssociatedObject property might be mixing concerns by combining the representation of a date-time value with operations related to the calendar model. This can lead to confusion and make the class harder to maintain.

Evaluation: AssociatedObject is necessary during deserialization with DateTimeSerializer in order get the timezone for the CalDateTime. However, the Calendar argument in CalDateTime CTORs is never used.

Action: ~~Consider separating the concerns of date-time representation and calendar operations. This might involve creating separate classes or methods for operations that act on CalDateTime instances.~~ Remove the the Calendar argument in CalDateTime CTORs.

Timing: #638

* Introduced new methods and test cases in `CalDateTimeTests.cs` to ensure correct behavior of `CalDateTime` properties and methods. * Updated various tests in `DeserializationTests.cs` and `RecurrenceTests.cs` to handle `CalDateTime` without time components and improve clarity. * Refined `IsAllDay` property in `CalendarEvent.cs` and updated methods in `UniqueComponent.cs` and `VTimeZone.cs` to use `CalDateTime` with time components. * Removed outdated comments and improved code formatting. * Enabled nullable reference types and updated `IDateTime` interface. * Added new methods and properties to `CalDateTime.cs` for better date and time management. * Refactored methods in `Period`, `RecurrencePatternEvaluator`, and `RecurringEvaluator` classes to handle `HasTime` property correctly. * Improved `DateTimeSerializer` class for better performance and handling of nullable types. * Added XML documentation and marked obsolete methods. Resolves ical-org#630 Resolves ical-org#633 Resolves ical-org#635 Resolves ical-org#636 Resolves ical-org#637

Depending on the time offset from local time to UTC the tests failed. Changed all date kinds to UTC.

Affected classes: * `CalDateTime` * `DateUtil` * `DateTimeSerializer`

…alizer` `CalDateTime.TzId` does no more read or write `CalDateTime.Parameters`

…me.Value` * Depending on `HasTime`, `Value` contains only the `DateTime.Date`, or the value including `DateTime.TimeOfDay`. * The timezone dermines whether `DateTimeKind.Utc`or `DateTimeKind.Unspecified`will be used.

minichma

I love the simplification that comes with removing the ambiguous DateTime member field and with removing DateTimeKind.Local! Didn't do a full review, only CalDateTime for now, but I think that's the core part anyways. Left a few comments with my thoughts, maybe not all relevant for this PR but for following ones.

minichma · 2024-11-19T21:03:53Z

Ical.Net/DataTypes/CalDateTime.cs

    public static IDateTime operator -(CalDateTime left, TimeSpan right)
    {
-        var copy = left.Copy<IDateTime>();
+        var copy = left.Copy<CalDateTime>();
+        if (Math.Abs(right.TotalDays % 1) > AlmostZeroEpsilon)


Maybe (right.Ticks % TimeSpan.TicksPerDay) != 0? This way we could avoid dealing with FP precision issues.

excellent, done

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-11-19T21:30:59Z

Ical.Net/Utility/DateUtil.cs

@@ -22,7 +22,7 @@ public static IDateTime EndOfDay(IDateTime dt)
        => StartOfDay(dt).AddDays(1).AddTicks(-1);

    public static DateTime GetSimpleDateTimeData(IDateTime dt)
-        => DateTime.SpecifyKind(dt.Value, dt.IsUtc ? DateTimeKind.Utc : DateTimeKind.Local);
+        => DateTime.SpecifyKind(dt.Value, dt.IsUtc ? DateTimeKind.Utc : DateTimeKind.Unspecified);


According to the improved CalDateTime, this would be as simple as => dt.Value.

right - done

minichma · 2024-11-19T21:58:51Z

Ical.Net/Utility/DateUtil.cs

I think, there are a few more places to deal with local date in this class.

The behavior of SimpleDateTimeToMatch is not easy to understand. Within the sln its only used for testing, so I'd suggest to move it to a test project, where the specific behavior might be helpful.

MatchTimeZone is only used in RecurrencePatternEvaluator. I'm pretty sure, that the way it deals with floating time is not the most natural one either (see GetOccurrences should not assume the system local date time #197). Probably we want to deal with that in a separate PR. Maybe move this method into RecurrencePatternEvaluator for now and/or add a comment that we should reconsider/fix how it deals with local time.

Done:

Move DateUtil.SimpleDateTimeToMatch as private to RecurrenceTests

Move DateUtil.MatchTimeZone as private to RecurrencePatternEvaluator

DateUtil is internal, so this shouldn't be breaking.

minichma · 2024-11-19T22:12:43Z

Ical.Net/DataTypes/CalDateTime.cs

+    /// the system's local timezone.
+    /// </param>
+    /// <param name="hasTime">Set to <see langword="true"/> (default), if the <see cref="DateTime.TimeOfDay"/> must be included.</param>
+    public CalDateTime(DateTime value, string? tzId, bool hasTime = true)


Having both, the Kind in the DateTime value, as well as the tzId, causes redundancy, that's not easy to understand. What, if we remove this constructor altogether? The user could easily call new CalDateTime(value).ToTimeZone(tzId), with way less ambiguity.

This will be back on the agenda when we implement floating date/time. See #647

minichma · 2024-11-19T22:15:58Z

Ical.Net/DataTypes/CalDateTime.cs

    public DateTime Date => Value.Date;

+    /// <inheritdoc cref="DateTime.TimeOfDay"/>
    public TimeSpan TimeOfDay => Value.TimeOfDay;


Isn't this obsolete, now that we have the TimeOnlyValue?

see task in #646

minichma · 2024-11-19T22:17:45Z

Ical.Net/DataTypes/CalDateTime.cs

    public int DayOfYear => Value.DayOfYear;

+    /// <inheritdoc cref="DateTime.Date"/>
    public DateTime Date => Value.Date;


Obsolete with DateOnlyValue?

see task in #646

minichma · 2024-11-19T22:20:03Z

Ical.Net/DataTypes/CalDateTime.cs

+    /// <summary>
+    /// Gets the underlying <see cref="DateOnlyValue"/> of <see cref="Value"/>.
+    /// </summary>
+    public DateOnly? DateOnlyValue => HasDate ? _dateOnly : null;


Maybe just name it Date, in replacement of the existing, now obsolete Date?

see task in #646

minichma · 2024-11-19T22:20:20Z

Ical.Net/DataTypes/CalDateTime.cs

+    /// <summary>
+    /// Gets the underlying <see cref="TimeOnlyValue"/> of <see cref="Value"/>.
+    /// </summary>
+    public TimeOnly? TimeOnlyValue => HasTime ? _timeOnly : null;


Just Time or TimeOfDay?

see task in #646

* `if (Math.Abs(right.TotalDays % 1) > AlmostZeroEpsilon)` is now `if ((right.Ticks % TimeSpan.TicksPerDay) != 0)` * `DateUtil.DateTime GetSimpleDateTimeData(IDateTime dt)` returns `dt.Value` * Move internal `DateUtil.SimpleDateTimeToMatch` as private to `RecurrenceTests` * Move internal `DateUtil.MatchTimeZone` as private to `RecurrencePatternEvaluator` * Create tasks in ical-org#646 and ical-org#647 for new PRs

Also refactor for SonarCloud complaints

sonarcloud · 2024-11-22T19:22:58Z

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 · 2024-11-25T19:15:26Z

@minichma Merged as you already reviewed twice :)

minichma · 2024-11-25T20:21:41Z

Sorry for not being able to review this any earlier. Anyhow, this is a significant improvement, congrats!

axunonb changed the title ~~Enhance Refactor CalDateTime and DateTimeSerializer~~ Enhance CalDateTime and DateTimeSerializer Nov 9, 2024

axunonb force-pushed the pr/caldatetime2 branch 2 times, most recently from 01de82a to 0743c68 Compare November 9, 2024 16:35

axunonb mentioned this pull request Nov 9, 2024

Refactor CalDateTime and DateTimeSerializer #631

Closed

axunonb force-pushed the pr/caldatetime2 branch from 0743c68 to a4cd5c0 Compare November 9, 2024 16:41

axunonb marked this pull request as ready for review November 9, 2024 16:43

axunonb requested a review from minichma November 9, 2024 16:43

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs Outdated Show resolved Hide resolved

axunonb force-pushed the pr/caldatetime2 branch from 08c7ff1 to 28df6fd Compare November 11, 2024 10:19

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/CalDateTime.cs Outdated Show resolved Hide resolved

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/CalDateTime.cs Outdated Show resolved Hide resolved

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/CalDateTime.cs Outdated Show resolved Hide resolved

axunonb force-pushed the pr/caldatetime2 branch from 69f6508 to a5b1bf5 Compare November 11, 2024 17:16

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/CalDateTime.cs Outdated Show resolved Hide resolved

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/CalDateTime.cs Outdated Show resolved Hide resolved

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/Period.cs Outdated Show resolved Hide resolved

axunonb commented Nov 11, 2024

View reviewed changes

Ical.Net/DataTypes/CalDateTime.cs Outdated Show resolved Hide resolved

axunonb force-pushed the pr/caldatetime2 branch 3 times, most recently from 7ff8e90 to edf6103 Compare November 12, 2024 08:54

minichma reviewed Nov 12, 2024

View reviewed changes

axunonb added the version 5 label Nov 18, 2024

axunonb force-pushed the pr/caldatetime2 branch from 1d15f23 to 54ee355 Compare November 18, 2024 08:38

Fix occasual failing unit test

e7f772f

Depending on the time offset from local time to UTC the tests failed. Changed all date kinds to UTC.

axunonb force-pushed the pr/caldatetime2 branch from 54ee355 to e7f772f Compare November 18, 2024 08:44

axunonb added 5 commits November 18, 2024 09:45

Make AlmostZeroEpsilon private const

b638e22

Replace DateTimeKind.Local with DateTimeKind.Unspecified

bfaa189

Affected classes: * `CalDateTime` * `DateUtil` * `DateTimeSerializer`

SOC: Move getting "TZID" from IParameterCollection to `DateTimeSeri…

5864b87

…alizer` `CalDateTime.TzId` does no more read or write `CalDateTime.Parameters`

Use backing fields DateOnly and TimeOnly to create the `CalDateTi…

82f5dba

…me.Value` * Depending on `HasTime`, `Value` contains only the `DateTime.Date`, or the value including `DateTime.TimeOfDay`. * The timezone dermines whether `DateTimeKind.Utc`or `DateTimeKind.Unspecified`will be used.

Remove the redundant Calendar argument to CTORs

80c2578

axunonb requested a review from minichma November 18, 2024 13:13

axunonb changed the base branch from main to version/v5 November 18, 2024 13:17

minichma mentioned this pull request Nov 19, 2024

RecurrencePattern.Until DateTime property should be a CalDateTime #588

Open

minichma reviewed Nov 19, 2024

View reviewed changes

axunonb added 2 commits November 21, 2024 00:08

Merge branch 'version/v5' into pr/caldatetime2

8f0ce8e

axunonb requested a review from minichma November 21, 2024 20:48

Merge branch 'version/v5' into pr/caldatetime2

712c392

axunonb changed the base branch from version/v5 to main November 22, 2024 08:26

Remove xmldoc for non-existing parameter "kind"

7bf9c3b

Also refactor for SonarCloud complaints

axunonb force-pushed the pr/caldatetime2 branch from b8a64e7 to 7bf9c3b Compare November 22, 2024 19:22

axunonb merged commit 796018e into ical-org:main Nov 25, 2024
3 checks passed

axunonb deleted the pr/caldatetime2 branch November 25, 2024 19:14

Enhance CalDateTime and DateTimeSerializer #638

Enhance CalDateTime and DateTimeSerializer #638

Conversation

axunonb commented Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axunonb Nov 12, 2024 • edited Loading

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

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

minichma commented Nov 12, 2024

axunonb commented Nov 13, 2024 • edited Loading

Use of NodaTime for Date and Time Representation

Necessity of IDateTime Interface

AssociatedObject Property

minichma left a 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

Choose a reason for hiding this comment

axunonb Nov 21, 2024 • edited Loading

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

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

sonarcloud bot commented Nov 22, 2024

Quality Gate passed

axunonb commented Nov 25, 2024

minichma commented Nov 25, 2024

axunonb commented Nov 9, 2024 •

edited

Loading

axunonb Nov 12, 2024 •

edited

Loading

axunonb commented Nov 13, 2024 •

edited

Loading

Use of `NodaTime` for Date and Time Representation

Necessity of `IDateTime` Interface

axunonb Nov 21, 2024 •

edited

Loading