Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CalDateTime Arithmetic Operations #672

Merged
merged 6 commits into from
Dec 22, 2024
Merged

CalDateTime Arithmetic Operations #672

merged 6 commits into from
Dec 22, 2024

Conversation

axunonb
Copy link
Collaborator

@axunonb axunonb commented Dec 16, 2024

  1. Fix CalDateTime Arithmetic Operations

    • Ensure TimeSpan add and subtract methods are reversible across DST transitions.
    • Consolidate all time-related arithmetic operations into a single method for consistency and maintainability.
    • Add related unit tests
  2. Remove TimeSpan operator overloads from CalDateTime
    Removed + and - operator overloads for CalDateTime that handled adding and subtracting TimeSpan objects

  3. Throw for invalid time span operations on date-only instance
    Throw when attempting to add/subtracting a time span to/from a date-only instance,
    and the time span is not a multiple of full days.

Resolves #670

- Ensure `TimeSpan` add and subtract methods are reversible across DST transitions.
- Consolidate all time-related arithmetic operations into a single method for consistency and maintainability.
- Add related unit tests
Removed `+` and `-` operator overloads for `CalDateTime` that handled
adding and subtracting `TimeSpan` objects
Throw when attempting to add/subtracting a time span to/from a date-only instance,
and the time span is not a multiple of full days.
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/DataTypes/CalDateTime.cs 78% 0 Missing and 8 partials ⚠️

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #672   +/-   ##
===================================
- Coverage    63%    63%   -0%     
===================================
  Files        99     99           
  Lines      4576   4550   -26     
  Branches   1079   1075    -4     
===================================
- Hits       2886   2863   -23     
  Misses     1240   1240           
+ Partials    450    447    -3     
Files with missing lines Coverage Δ
Ical.Net/DataTypes/CalDateTime.cs 88% <78%> (-<1%) ⬇️

@axunonb axunonb requested a review from minichma December 16, 2024 19:52
@axunonb axunonb marked this pull request as ready for review December 16, 2024 19:52
minichma
minichma previously approved these changes Dec 21, 2024
Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Added some comments that might a subject rather for a follow-up PR rather than for this one.

@minichma
Copy link
Collaborator

The public TimeSpan Subtract(IDateTime dt) => AsUtc - dt.AsUtc should probably be updated as part of this PR as well to be able to deal with floating time.

@axunonb
Copy link
Collaborator Author

axunonb commented Dec 22, 2024

The public TimeSpan Subtract(IDateTime dt) => AsUtc - dt.AsUtc should probably be updated as part of this PR as well to be able to deal with floating time.

This should already work. AsUtc calls ToTimeZone(UtcTzId).Value, which also covers IsFloating.

@axunonb axunonb merged commit 6f7998f into main Dec 22, 2024
6 of 8 checks passed
@axunonb axunonb deleted the wip/axunonb/cdt-arithmetic branch December 22, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CalDateTime arithmetic operations should assert (t + d - t == d)
2 participants