Refactor to use DateOnly and TimeOnly for date/time handling #658

axunonb · 2024-11-30T21:27:34Z

Increased CalDateTime immutability

CalDateTime:

Date is of type DateOnly (breaking vs. v4)
Make UtzTzId a public string
Remove DateOnlyValue
Remove HasDate (always true) (breaking vs. v4)
Remove Millisecond (breaking vs. v4)
Remove Ticks (breaking vs. v4)
Remove overload CalDateTime(int year, int month, int day, string? tzId) (breaking vs. v4)
HasTime only has a getter (breaking vs. v4)
Time is a getter of type TimeOnly. Values are always rounded to the nearest second (breaking vs. v4)
AddMilliseconds takes double as argument, like DateTime.AddMilliseconds (breaking vs. v4)
Remove TimeOnlyValue
Value only has a getter (breaking vs. v4)
TzId only has a getter (breaking vs. v4)
Remove AsSystemLocal (breaking vs. v4)
Remove AsDateTimeOffset (breaking vs. v4)
Remove AddMilliseconds (breaking vs. v4)
Remove AddTicks (breaking vs. v4)
Remove unused TimeSpan? operator -(CalDateTime? left, IDateTime? right) (breaking vs. v4)
Update Equality operators for comparing to floating date/time (breaking vs. v4)

IDateTime (breaking vs. v4):

Remove AsSystemLocal (breaking vs. v4)`
Remove AsDateTimeOffset (breaking vs. v4)`
Remove AddMilliseconds (breaking vs. v4)
Remove AddTicks (breaking vs. v4)
Remove Millisecond (breaking vs. v4)
Remove Ticks (breaking vs. v4)
Add explicit bool IsFloating (implicit TzId == null)
Date is a getter of type DateOnly
Time is a getter of type TimeOnly
HasTime only has a getter
Remove HasDate (always true) (breaking vs. v4)
Value only has a getter
TzId only has a getter
ToTimeZone(string?) argument is a NRT

Miscellaneous:

CTORs with DateTime arguments persist. Actually they are also comfortable, and now consistently processed
Keep IDateTime - maybe in another PR
Transitioned from DateTime to DateOnly and TimeOnly across the codebase for improved date and time handling.
Updated methods, properties, and tests to ensure compatibility with the new approach.
Updated xmldoc

Resolves #656
Resolves #662

Increased `CalDateTime` immutability CalDateTime: * `Date` is of type `DateOnly` (breaking vs. v4) * Make `UtzTzId` a public string * Remove `DateOnlyValue` * Remove `HasDate` (always true) (breaking vs. v4) * Remove overload CalDateTime(int year, int month, int day, string? tzId) (breaking vs. v4) * `HasTime` only has a getter (breaking vs. v4) * `Time` is a getter of type `TimeOnly`. Values are always rounded to the nearest second (breaking vs. v4) * Remove `TimeOnlyValue` * `Value` only has a getter (breaking vs. v4) * `TzId` only has a getter (breaking vs. v4) * Remove `AsSystemLocal` (breaking vs. v4) * Remove unused `TimeSpan? operator -(CalDateTime? left, IDateTime? right)` (breaking vs. v4) * Update Equality operators for comparing to floating date/time (breaking vs. v4) IDateTime (breaking vs. v4): * Remove `AsSystemLocal` * Add explicit bool IsFloating (implicit TzId == null) * `Date` is a getter of type `DateOnly` * `Time` is a getter of type `TimeOnly` * `HasTime` only has a getter * Remove `HasDate` * `Value` only has a getter * `TzId` only has a getter * `ToTimeZone(string?)` argument is a NRT Miscellaneous: * CTORs with `DateTime` arguments persist. Actually they are also comfortable, and now consistently processed * Keep `IDateTime` - maybe in another PR * Transitioned from DateTime to DateOnly and TimeOnly across the codebase for improved date and time handling. * Updated methods, properties, and tests to ensure compatibility with the new approach. * Updated xmldoc

Ical.Net.Tests/CalDateTimeTests.cs

Ical.Net.Tests/ComponentTest.cs

Ical.Net.Tests/ProgramTest.cs

Ical.Net.Tests/RecurrenceTests.cs

…anged Remove fallback to system's local timezone for floating date/time

github-advanced-security · 2024-12-02T13:48:25Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Ical.Net/DataTypes/IDateTime.cs

axunonb · 2024-12-02T14:38:19Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs


        return dt1.IsUtc
            ? new CalDateTime(copy.AsUtc)
-            : new CalDateTime(copy.AsSystemLocal);


Isn't the code in MatchTimeZone(IDateTime dt1, IDateTime dt2) completely useless?

Its not the most elegant code, but it aims to adjust UNTIL, which usually is specified in UTC, to DTSTART's time zone, which usually isn't in UTC. This allows checking comparing the adjusted UNTIL with the generated recurrences without considering the time zone again. So the functionality is needed one way or the other.

That's what I thought, but the way it is implemented looks like a noop. Then I removed the method invocation temporarily with no effect on unit tests. So this is just a hint re recurrences.

I could easily be wrong, or relevant tests are missing. We'd need a test with UNTIL exactly at the last recurrence and then run the test with a TZ with positive offset and with one with a negative offset. In any case the last recurrence should be returned.

The 2 arguments to MatchTimeZone(IDateTime dt1, IDateTime dt2) always have the same timezone, so there is nothing to adjust? [Edit] The method in effect is r.Until = r.Until.

if (r.Until != DateTime.MinValue) { r.Until = MatchTimeZone(referenceDate, new CalDateTime(r.Until, referenceDate.TzId)).Value; }

You are fully right, its a noop. But I strongly believe that something's wrong there, because the conversion needs to be done somewhere. Should be investigated separately. Maybe the translation between UTC and tzid happened in the constructor of CalDateTime and was lost in one of the previous PRs?

Created #663 to track

Resolves ical-org#660

…t null Reasoning: DATE cannot have a timezone

Ical.Net/Calendar.cs

minichma · 2024-12-02T19:45:02Z

Just noted, that I didn't submit my review comments from 2d ago, so please read my comments in the context of last Sat.

Ical.Net/DataTypes/Trigger.cs

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-11-30T23:01:51Z

Ical.Net/DataTypes/CalDateTime.cs

+            return false;
+        }
+
+        if (left.IsFloating || right.IsFloating)


Suggest to add a check if IsFloating equals. I.e.

if (left.IsFloating != right.IsFloating) return false;

On the first glance I thought the proposal worked, but it doesn't.

If we don't add this check, we could consider instances as equal even if one is floating and the other has a TZ set, which seems wrong. What would be the reason for that behavior?

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-11-30T23:22:25Z

Ical.Net/Evaluation/RecurrenceUtil.cs

-        new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);
+    public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults)
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


Be careful, Time could be 0:00 in which case we'd end up with a negative time.

InvalidOperationException: see below

Not sure, I understand. I think the problem still exists. If someone specifies a dt e.g. 20241203T000000, which is legit, then this code would lead to 20241203 T -000001.

I think, we would introduce a new bug here and should consider fixing it right away.

minichma · 2024-12-02T19:39:45Z

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs


        return dt1.IsUtc
            ? new CalDateTime(copy.AsUtc)
-            : new CalDateTime(copy.AsSystemLocal);


Its not the most elegant code, but it aims to adjust UNTIL, which usually is specified in UTC, to DTSTART's time zone, which usually isn't in UTC. This allows checking comparing the adjusted UNTIL with the generated recurrences without considering the time zone again. So the functionality is needed one way or the other.

Ical.Net/Calendar.cs

minichma · 2024-12-02T19:47:01Z

This is a great improvement regarding readability and maintainability!

axunonb · 2024-12-04T23:46:20Z

@minichma PR comments should all all be resolved or addressed

minichma · 2024-12-05T05:44:49Z

I'm on a business trip until Sat. Not sure, whether I'll find the time until then.

minichma · 2024-12-05T15:35:56Z

Ical.Net/Calendar.cs

    public virtual HashSet<Occurrence> GetOccurrences(IDateTime dt)
-        => GetOccurrences<IRecurringComponent>(new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)));
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


This is a new issue introduced in this PR, so shouldn't we fix it right away? The behavior is also somewhat different from the previous one. In the previous version we just truncated the time, in the new one we keep the time, if set, and observe the period until the same time next day. Is this intentional?

Ical.Net/DataTypes/CalDateTime.cs

minichma · 2024-12-05T15:56:34Z

Ical.Net/DataTypes/CalDateTime.cs

-        if (!dt.HasTime && hours % 24 > 0)
+        var copy = Copy<CalDateTime>();
+        var newValue = copy.Value.AddHours(hours);
+        if (!copy.HasTime && (hours % 24 == 0))


We should consider (in a subsequent PR) to remove the special handling of multiples of 24. This is the kind of magic we had in v4 with midnight. A user wouldn't understand, why adding multiple of 24h behaves differently than adding other values. I suggest to either disallow adding hours to date-only instances altogether (preferred) , or, to always convert to date-time.

issues #661 and #662 already exists since last review

Ical.Net/Evaluation/RecurrencePatternEvaluator.cs

minichma · 2024-12-05T16:05:32Z

Ical.Net/Evaluation/RecurrenceUtil.cs

-        new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);
+    public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults)
+    {
+        var endTimeOnly = dt.Time?.Add(TimeSpan.FromSeconds(-1));


I think, we would introduce a new bug here and should consider fixing it right away.

axunonb · 2024-12-05T18:34:18Z

introduce a new bug here and should consider fixing it right away

~~Agree about this should be fixed (#662) with another PR.~~
The fix was trivial, so it's done in 5956fb9

It's existing code logic though:

ical.net/Ical.Net/Evaluation/RecurrenceUtil.cs

Lines 16 to 17 in add58a9

    
           public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults) => GetOccurrences(recurrable, 
        
               new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)), includeReferenceDateInResults);

[Edit]
The remaining time adjustments were only found in class Calendar.
The adjustments are redundant, because
public static HashSet<Occurrence> RecurrenceUtil.GetOccurrences(IRecurrable recurrable, IDateTime periodStart, IDateTime periodEnd, bool includeReferenceDateInResults)
uses precise "LessThan" for comparing end date/times
This is now also cleaned-up with 5956fb9

Removed the `endTimeOnly` variable and its usage in the `GetOccurrences` method. The code now directly creates the `CalDateTime` object for the end of the period without adjusting the time component. This change streamlines the code and avoids potential issues related to time adjustments.

axunonb · 2024-12-05T19:33:56Z

Comments resolved or tracked by issues

The `GetOccurrences` methods in `Calendar.cs` have been simplified by removing the code that adjusted the end time by subtracting one second or one tick. The methods now directly use the start date and the date one day after the start date without adjusting the time component. This change simplifies the logic and removes unnecessary time adjustments. The adjustments are redudant, because public static HashSet<Occurrence> RecurrenceUtil.GetOccurrences(IRecurrable recurrable, IDateTime periodStart, IDateTime periodEnd, bool includeReferenceDateInResults) uses precise "LessThan" for comparing end date/times Resolves ical-org#662

sonarqubecloud · 2024-12-05T22:49:52Z

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-12-05T23:01:19Z

The remaining time adjustments were only found in class Calendar.

The adjustments are redundant, because calls finally go to
public static HashSet<Occurrence> RecurrenceUtil.GetOccurrences(IRecurrable recurrable, IDateTime periodStart, IDateTime periodEnd, bool includeReferenceDateInResults)
which uses precise "LessThan" for comparing end date/times.

minichma · 2024-12-06T16:53:30Z

The fix was trivial, so it's done in 5956fb9

It's existing code logic though:

Great, thank you! The difference to the previous code is, that the previous version calculated 23:59:59 the previous day, but the new code calculated -0:00:01 the same day. So now its certainly better.

minichma

Great improvement!

CAPCHIK · 2025-06-18T06:12:28Z

Ical.Net/DataTypes/CalDateTime.cs

-    /// If the <see cref="TzId"/> is <see langword="null"/> or does not represent an
-    /// IANA or other well-known timezone ID, the system's local timezone will be used.
-    /// </remarks>
-    public DateTimeOffset AsDateTimeOffset => DateUtil.ToZonedDateTimeLeniently(Value, TzId).ToDateTimeOffset();


@axunonb Hi!

Could you explain why AsDateTimeOffset was removed? It was useful in v4, and DateTimeOffset is still used in ToString.

How can I get a DateTimeOffset from CalDateTime now? DateUtil is internal, and in v5, it seems I have to use the same logic to extract the timezone offset from NodaTime.

I posted this as a comment because I was hoping to find the reason for the change in the commit history. Maybe it would be better to raise this as a separate issue?

@CAPCHIK The reasons for the redesign are given at the top of this PR. This was the decision at this time.
We're currently not yet happy about CalDateTime as it is now,. All timezone related methods should go into a separate class, maybe as extension methods.
Any changes to CalDateTime should happen after the final design is clear. We welcome every helping hand with ical.net.
The new ical.net v5 release reflects the current WIP for CalDatetime.

I have to use the same logic to extract the timezone offset from NodaTime.

Yes, for v5.0.0

Thanks for the explanation, I will try to find a way to help in the development.

axunonb force-pushed the pr/refactor-idatetime branch 3 times, most recently from 39c43e4 to 7a72ba1 Compare November 30, 2024 22:56

axunonb force-pushed the pr/refactor-idatetime branch from 7a72ba1 to 7463f67 Compare December 1, 2024 21:01

Remove AsDateTimeOffset from IDateTime and CalDateTime

67cda7c

axunonb force-pushed the pr/refactor-idatetime branch from ba7eb3e to 67cda7c Compare December 2, 2024 09:02