Enhance CalDateTime and DateTimeSerializer by axunonb · Pull Request #638 · ical-org/ical.net

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

-                if (isEmpty)
+                if (string.IsNullOrWhiteSpace(value))
                {
+                    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

 {
+    // 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.

* `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

sonarqubecloud · 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