Remove date and time arithmetic from CalDateTime#930
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## version/6.0 #930 +/- ##
=============================================
+ Coverage 70.2% 70.3% +0.1%
=============================================
Files 118 118
Lines 4645 4622 -23
Branches 1094 1086 -8
=============================================
- Hits 3260 3247 -13
+ Misses 1025 1013 -12
- Partials 360 362 +2
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
We already have a couple of breaking changes in terms of
So this step is just kind of finalizing the direction we took in v6. As you know I was hoping for an easy migration path from v5 to v6. Now v6 requires a direct dependency on NodaTime and users will have to learn it and rewrite their current |
| } | ||
| } | ||
|
|
||
| #if NET6_0_OR_GREATER |
There was a problem hiding this comment.
Could be removed in favor of NodaTime.
| /// </summary> | ||
| public LocalTime? Time => _localTime; | ||
|
|
||
| #if NET6_0_OR_GREATER |
There was a problem hiding this comment.
Could be removed in favor of NodaTime.
There was a problem hiding this comment.
I think it is better to keep these. These are simple convenience methods that do not add complexity. For those who refuse to use NodaTime, these could be more useful than being stuck with only DateTime.
There was a problem hiding this comment.
Those ical.net users who refuse to use NodaTime will have a difficult time ahead of them 😉
| { | ||
| return ToLocalDateTime().InUtc(); | ||
| } | ||
| else |
| { | ||
| return ToLocalDateTime().InZoneLeniently(timeZone); | ||
| } | ||
| else |
|
|
||
| public ZonedDateTime ToZonedDateTime(string zoneId) | ||
| { | ||
| return ToZonedDateTime(DateUtil.GetZone(zoneId)); |
There was a problem hiding this comment.
Maybe use expression body?
| { | ||
| return ToLocalDateTime().InZoneLeniently(timeZone); | ||
| } | ||
| else |
| @@ -464,103 +464,6 @@ | |||
| return new(ToZonedDateTime(otherTzId)); | |||
There was a problem hiding this comment.
Add a comment that this drops the timezone offset and converting back to ZonedDatetime will not resolve the same value?
|
|
||
| namespace Ical.Net.Utility; | ||
|
|
||
| internal static class DateUtil |
There was a problem hiding this comment.
Do we still need this class?
WeekOfMonth(DateTime d) has just one reference
| } | ||
|
|
||
| public NodaTime.Period ToPeriod() | ||
| public static Duration FromPeriod(NodaTime.Period p) |
There was a problem hiding this comment.
Not for this PR: Should we remove the Period ambiguity between NodaTime and ical.net by using another name, e.g. CalPeriod?
There was a problem hiding this comment.
I would like all NodaTime name overlaps to be renamed - CalPeriod, CalDuration, and any others.
I have been looking at #749 and handling time zones defined in the calendar. This will remove the last major reference to
This and the many redundant |
maknapp
force-pushed
the
maknapp/remove-caldatetime-arithmetic
branch
from
889dd85 to
3a3fab1
Compare
|
* Remove time arithmetic from CalDateTime * Remove date arithmetic from CalDateTime * Add tests for Duration
|
@maknapp After merging this PR I performed a rebase of |
2 participants
From discussion in #849 and #854, one way to resolve math issues is to leave math to
NodaTime(orSystem.DateTime). This removes allAdd*()methods fromCalDateTimeand replaces usage by converting toNodaTimeand back.Doing this adds more support towards having
NodaTimein the public API. Making it easy to convert to and fromNodaTimetypes is important.While this change does make sense, it does complicate things significantly for users who use
Add*(). For those unfamiliar withNodaTime, they would have to learnNodaTimewhile also understanding thatCalDateTimecan hold date, datetime, or datetime with time zone without offset.CalDateTimewill need to be converted to the correctNodaTimetype based on the data it holds and what date/time units are being added. This will need to be well documented.Fixes #849