Skip to content

Conversation

@axunonb
Copy link
Collaborator

@axunonb axunonb commented May 29, 2025

  • Added internal clss CalDateTimeEvaluator
  • Added lightweight internal CalDateTimeZoned which is returned from CalDateTimeEvaluator methods
  • CalDateTime changes with low impact on existing code:
    • Mark all comparison operators as obsolete with error
    • Mark Utc property as obsolete with error
    • Move all arithmetic methods and comparison methods to new CalDateTimeExtensions. The extension methods use the internal CalDateTimeZoned behind the scene.
  • Refactor Period to be usable with CalDateTimeZoned internally
  • Added Microsoft.Bcl.HashCode for simplified HashCode generation with netstandard2.x

Resolves #677
Resolves #737

@codecov
Copy link

codecov bot commented May 29, 2025

Codecov Report

Attention: Patch coverage is 96.50000% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/DataTypes/Period.cs 88% 0 Missing and 6 partials ⚠️
Ical.Net/CalendarComponents/RecurringComponent.cs 0% 1 Missing ⚠️

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

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #809   +/-   ##
===================================
+ Coverage    67%    68%   +1%     
===================================
  Files       106    109    +3     
  Lines      4206   4284   +78     
  Branches    947    975   +28     
===================================
+ Hits       2822   2917   +95     
+ Misses     1044   1040    -4     
+ Partials    340    327   -13     
Files with missing lines Coverage Δ
Ical.Net/CalendarComponents/FreeBusy.cs 48% <100%> (ø)
Ical.Net/CalendarComponents/VTimeZone.cs 72% <ø> (ø)
Ical.Net/CalendarExtensions.cs 100% <ø> (ø)
Ical.Net/CollectionExtensions.cs 100% <100%> (ø)
Ical.Net/DataTypes/CalDateTime.cs 94% <100%> (+7%) ⬆️
Ical.Net/DataTypes/Duration.cs 77% <100%> (ø)
Ical.Net/Evaluation/CalDateTimeEvaluator.cs 100% <100%> (ø)
Ical.Net/Evaluation/CalDateTimeExtensions.cs 100% <100%> (ø)
Ical.Net/Evaluation/CalDateTimeZoned.cs 100% <100%> (ø)
Ical.Net/Evaluation/EventEvaluator.cs 90% <100%> (ø)
... and 5 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch 6 times, most recently from 8cabdc5 to b9a526a Compare May 30, 2025 10:08
@axunonb
Copy link
Collaborator Author

axunonb commented May 30, 2025

@minichma Currently, CalDateTime still contains the comparison and arithmetic methods (flagged with a comment), that should eventually be removed.
Should we keep them for v5, remove, or flag as obsolete? What do you think?
CalDateTimeZoned could also be returned with GetOccurrences etc.
(Also, the unit tests are not yet adopted.)

@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch from b9a526a to bb192a9 Compare May 30, 2025 15:04
@minichma
Copy link
Collaborator

@minichma Currently, CalDateTime still contains the comparison and arithmetic methods (flagged with a comment), that should eventually be removed. Should we keep them for v5, remove, or flag as obsolete? What do you think? CalDateTimeZoned could also be returned with GetOccurrences etc. (Also, the unit tests are not yet adopted.)

If we have a new type particularly for doing arithmetics, etc., then I feel it would be confusing to have all the methods also available on CalDateTime. But removing them from there will be a big deal, because CalDateTimeZoned would have to be introduced everywhere.

Not sure I'd introduce a new type (i.e. CalDateTimeZoned) at all. Wouldn't it be an option just to use NodaTime's ZonedDateTime instead. Problems are that it can't represent a date-only or a floating time value. Floating time could be represented by introducing a dedicated timezone class for that purpose. Regarding date-only, could it be feasible that we don't represent those as date-only during evaluation at all? Not sure. Unfortunately I don't have the time right now to go into more detail. Anyhow, quite a challenge for v5, should we want to release it any time soon. I guess it would be more reasonable to postpone this whole topic to v6.

@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch 3 times, most recently from f441c5f to f839fc3 Compare June 1, 2025 21:13
* Added `CalDateTimeEvaluator`
* Added lightweight `CalDateTimeZoned` which is returned from `CalDateTimeEvaluator` methods
* Updated existing `CalDateTime` comparion and arithmethic related methods to use `CalDateTimeEvaluator`
* Added `Microsoft.Bcl.HashCode` for simplified HashCode generation with `netstandard2.x`
@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch from f839fc3 to 919e8f4 Compare June 1, 2025 21:37
@axunonb
Copy link
Collaborator Author

axunonb commented Jun 1, 2025

If we have a new type particularly for doing arithmetics, etc., then I feel it would be confusing to have all the methods also available on CalDateTime.

The methods in CalDateTime would be marked as obsolete with a hint to CalDateTimeZoned for easier migration.
CalDateTimeZoned can be internal with the latest commits. CalDateTimeExtensions be for zoned arithmetics and comparisons, as you suggested.

@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch from b4bd390 to 3dbdc95 Compare June 2, 2025 10:46
@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch from 3dbdc95 to 2375512 Compare June 2, 2025 11:38
…rison methods to CalDateTimeExtensions

Obsolete CalDateTime.AsUtc
@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch 6 times, most recently from 47ebb03 to 64d8545 Compare June 3, 2025 14:07
Update `EventEvaluator` to use `CalDateTimeZoned` with `Period`
No change zu the public API
Add unit test AmbiguousLocalTime_WithShortDurationOfRecurrence which resolves #737
Make CalDateTimeEvaluator.ToString use existing ZoneDateTime
@axunonb axunonb force-pushed the wip/axunonb/pr/datetime-evaluation branch from 64d8545 to 1be4a17 Compare June 3, 2025 15:48
Update `EventEvaluator` to use `CalDateTimeZoned` with `Period`
No change zu the public API
Add unit test AmbiguousLocalTime_WithShortDurationOfRecurrence which resolves #737
Make CalDateTimeEvaluator.ToString use existing ZoneDateTime
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 4, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean-up redundant implicit/arithmetic operators in CalDateTime Incorrect timezone calculation if short recurrence falls into an ambiguous local time

3 participants