Simplify and cleanup CalDateTime

[maknapp]

The intent here is to remove more from CalDateTime while keeping it simple to use. See individual commits for changes. I can change to separate PRs if that is preferred.

Breaking Changes:

1. ToTimeZone(string?) removed
2. AsUtc property removed
3. Value property removed
4. CalDateTime(DateTime, string? tzId, bool? hasTime = null) removed
5. CalDateTime(DateTime, bool? hasTime = null) removed
6. CalDateTime(DateOnly date) -> CalDateTime FromDateOnly(DateOnly date) (and only .NET 6+)
7. CalDateTime(DateOnly date, TimeOnly? time, string? tzId = null) removed
8. Hour, Minute, Second properties removed

[maknapp]

@axunonb version/6.0 is protected and never supposed to move. Why was version/6.0 rebased? Losing all commit dates... can we change it back?

[axunonb]

See #930 (comment)
There is a branch version/6.0-backkup that I created before the rebase.
If you think the commit dates are important, we can go back to this backup.
The author dates are untouched - see git log --pretty=fuller

[maknapp]

I remember reading that comment now and was confused then but skipped over it and forgot to go back. I do not like changing history of any shared/major branches. Looking back to #854 (comment), I was assuming this meant that version/6.0 was eventually going to be renamed to master or merged with a merge commit, not rebased. Rebasing a large branch like this repeatedly to keep up-to-date with master also makes links and history in github more confusing. I would strongly prefer to go back to the original version/6.0 branch and never rebase it.

[axunonb]

Sure, feel free to recreate branch version/6.0 from version/6.0-backkup the way you prefer. The latest 3 commit are missing in the backup.

[maknapp]

I cannot force push to version/6.0 branch because it is restricted. I pushed a v6 branch which is the original with 2 of the latest commits. The encoding commit 58494b4 is a noop for me and just produces an empty commit.

[axunonb]

cannot force push to version/6.0 branch

The assumption was you can as a maintainer, sorry. You're now entitled to force push, while it is generally blocked.
Btw. would like to become an owner? Would make sense.

[maknapp]

Still blocked with:

! [remote rejected] version/6.0 -> version/6.0 (protected branch hook declined)"

We could just switch to using "v6" (shorter name is better?) and mark it protected.

Btw. would like to become an owner?

Sure, that would work too.

[axunonb]

@maknapp It would be helpful to rebase this branch before the review.

[maknapp]

See previous comment. I still cannot force push version/6.0 branch to revert it back to the previous base commit. This PR is based off of the original version/6.0 branch.

[axunonb]

See previous comment

That's the setting that should allow you to force-push. You're also admin of the repo. Maybe try again?

[maknapp]

Okay pushing worked now. I think everything is back to what it was except commit 58494b4 which was empty for me. Verify your files are still UTF-8 after switching to current version/6.0 branch.

[axunonb]

files are still UTF-8 after switching to current version/6.0 branch

git ls-files | ForEach-Object { $b=[IO.File]::ReadAllBytes($_); $e=if($b.Length-ge 3-and $b[0] -eq 0xEF-and $b[1] -eq 0xBB-and $b[2] -eq 0xBF){"UTF-8-BOM"}elseif($b.Length-ge 2-and $b[0] -eq 0xFF-and $b[1] -eq 0xFE){"UTF-16LE"}elseif($b.Length-ge 2-and $b[0] -eq 0xFE-and $b[1] -eq 0xFF){"UTF-16BE"}else{try{[Text.UTF8Encoding]::new($false,$true).GetString($b)|Out-Null;"UTF-8"}catch{"OTHER"}}; if($e -ne "UTF-8"){[PSCustomObject]@{Encoding=$e;File=$_}} } | Format-Table -AutoSize

reported no files that are not UTF-8, so all are 'clean'. Thanks!

[Edit]
With this chore done, we should remove the backup branch, right?

#931 isn't included in v6 now, while it should. Maybe fix in this PR. See #936

[axunonb]

The breaking public API changes with no deprecation step should be clearly called out with the break in the PR description. This will be helpful for any changelog/migration docs.

[axunonb]

The breaking changes I collected so far are:

• ToTimeZone(string?) removed
• AsUtc property removed
• Value property removed
• CalDateTime(DateTime, bool hasTime, ...) constructor shape changed
• CalDateTime(ZonedDateTime) / CalDateTime(Instant) constructors replaced by factories

[maknapp]

Updated PR with breaking changes from main branch (NodaTime constructors are not on main).

[axunonb]

If DATE creation is moved to a static factory like with this method, it should follow the From* / To* convention already in use. The PR description mentions this but doesn't yet clarify the chosen name(s).

[maknapp]

I have been thinking about different options for this. It definitely cannot be FromDateTime() because it does not say a DATE value is being created. Some options:

// Clearly states the type and value used; will saying date-time-date confuse people?
public static CalDateTime FromDateTimeDate(DateTime value);

// Shortest name; implies date only but someone could assume time is included
public static CalDateTime FromDate(DateTime value);

// Change DateOnly constructor to static too?; Might confuse people into thinking value is DateOnly
public static CalDateTime FromDateOnly(DateTime value);
public static CalDateTime FromDateOnly(DateOnly value);

// Longest name; explicitly says type and that it will be a DATE value
public static CalDateTime FromDateTimeDateOnly(DateTime value);

Which do you like best, or do you have another name in mind?

[axunonb]

I'd say the cleanest approach is FromDate(...) with overloads:

The name describes the result (a DATE value), not the input type — which is the most important thing to communicate. It also mirrors the BCL precedent of DateOnly.FromDateTime(), where users already understand the pattern of "create this date-focused type from that input". The overload family removes any residual ambiguity:

// Ical.Net\DataTypes\CalDateTime.cs
public static CalDateTime FromDate(int year, int month, int day)
    => new CalDateTime(new LocalDate(year, month, day), null, null);

public static CalDateTime FromDate(DateTime value)
    => new CalDateTime(LocalDate.FromDateTime(value), null, null);

public static CalDateTime FromDate(LocalDate value)
    => new CalDateTime(value, null, null);

#if NET6_0_OR_GREATER
public static CalDateTime FromDate(DateOnly value)
    => new CalDateTime(value.ToLocalDate(), null, null);
#endif

Once a user sees any one overload, all others are predictable.

Another consideration:

If this factory lands, the DateTime DATE-TIME constructor could become a paired FromDateTime(...) static factory:

public static CalDateTime FromDateTime(DateTime value, string? tzId = null) { ... }

That would give a symmetric, discoverable pair: FromDate for RFC 5545 DATE, FromDateTime for RFC 5545 DATE-TIME - directly matching the spec's own type names.

Would you follow this idea?

[maknapp]

FromDate goes against BCL precedent, not the same. All From* static constructors that I can think of describe the input type to convert from, never the result (a DATE value). I cannot think of any types with multiple FromX overloads like this.

I would like converting the DateTime constructor to CalDateTime.FromDateTime(). It is generally not good to have both.

If we wanted to match the BCL and NodaTime, it should be just FromDateTimeDate with no overloads. That would be most similar to things like DateTimeOffset.FromUnixTime* or LocalTime.From*.

public static CalDateTime FromDateTimeDate(DateTime value);
public static CalDateTime FromDateTime(DateTime value);
public static CalDateTime FromDateOnly(DateOnly value);
public static CalDateTime FromInstant(Instant value);

[axunonb]

From* describes the input - my wrong.
So yes, then this makes sense.

[axunonb]

AsZonedOrDefault(DateTimeZone) has an inconsistent name relative to ToZonedDateTime().
As we're tidying the API, this might be a good time to rename it to something like ToZonedDateTimeOrFloating?

[maknapp]

I named this completely different on purpose to prevent people from confusing the intention of the argument. The time zone argument for the ToZonedDateTime methods is for converting to that time zone, whereas this one only adds the specified time zone if there is none.

I am open to other names, but I do not like ToZonedDateTimeOrFloating because my first thought is the result can somehow be floating.

[axunonb]

Maybe ToZonedDateTimeWithDefault? Else leave as is.

[maknapp]

Another option is to remove all ToZonedDateTime() and just rename AsZonedOrDefault() to ToZonedDateTime(). This would remove all ways to directly convert to another time zone. Users can make their own extension methods if they convert time zones all the time.

Will people still get confused and think ToZonedDateTime(DateTimeZone defaultZone) will always output a value in that time zone? Would it still need to be named something like ToZonedDateTimeWithDefault()?

dt.ToZonedDateTime() == dt.AsZonedOrDefault(DateTimeZone.Utc);

dt.ToZonedDateTime(dateTimeZone) == dt.AsZonedOrDefault(dateTimeZone).WithZone(dateTimeZone);

// This one is only used in tests and mostly to prevent having to change tests too much
dt.ToZonedDateTime("America/New_York") == dt.AsZonedOrDefault(tzdb["America/New_York"]).WithZone(tzdb["America/New_York"]);

[axunonb]

// Current ToZonedDateTime(DateTimeZone) — ALWAYS converts to the target zone
dt.ToZonedDateTime(americaNewYork); // result is always in America/New_York

// AsZonedOrDefault — only uses the zone if floating
dt.AsZonedOrDefault(americaNewYork); // result MAY NOT be in America/New_York

User may expect a conversion-to-zone, not a fallback-if-floating.

[axunonb]

Nice! See some mostly minor inline comments.

As for SonarCloud "Refactor 'GetHashCode' to not reference mutable fields." this is unfortunately the case for many other classes. Can be kept unchanged for now.

Maybe have a quick look at the Codecov Report, especially for CalDateTime decreasing coverage.

[Edit re CalDateTime coverage]:
public ZonedDateTime ToZonedDateTime(): Would be good to cover InvalidOperationException for _tzId == null?
public CalDateTime(LocalDate date) cover ArgumentOutOfRangeException?
The other uncovered methods are related to DateOnly / TimeOnly - not strictly necessary to cover

[codecov]

Codecov Report

❌ Patch coverage is 81.70732% with 15 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Ical.Net/Evaluation/EventEvaluator.cs	60.0%	2 Missing and 2 partials ⚠️
Ical.Net/Evaluation/TodoEvaluator.cs	0.0%	3 Missing ⚠️
Ical.Net/Calendar.cs	33.3%	0 Missing and 2 partials ⚠️
Ical.Net/CalendarComponents/Alarm.cs	0.0%	2 Missing ⚠️
Ical.Net/CollectionExtensions.cs	0.0%	1 Missing ⚠️
Ical.Net/DataTypes/FreeBusyEntry.cs	85.7%	0 Missing and 1 partial ⚠️
Ical.Net/DataTypes/Occurrence.cs	50.0%	1 Missing ⚠️
Ical.Net/DataTypes/Trigger.cs	0.0%	1 Missing ⚠️

@@             Coverage Diff             @@
##           version/6.0    #934   +/-   ##
===========================================
  Coverage         70.3%   70.3%           
===========================================
  Files              118     118           
  Lines             4622    4601   -21     
  Branches          1086    1076   -10     
===========================================
- Hits              3247    3233   -14     
+ Misses            1013    1009    -4     
+ Partials           362     359    -3     

Files with missing lines	Coverage Δ
Ical.Net/CalendarComponents/FreeBusy.cs	79.1% <100.0%> (+1.6%)	⬆️
Ical.Net/CalendarComponents/VTimeZone.cs	72.6% <100.0%> (ø)
Ical.Net/DataTypes/CalDateTime.cs	100.0% <100.0%> (+9.9%)	⬆️
Ical.Net/DataTypes/Period.cs	96.1% <100.0%> (ø)
Ical.Net/DataTypes/RecurrenceIdentifier.cs	100.0% <100.0%> (ø)
Ical.Net/Evaluation/RecurrenceRuleEvaluator.cs	93.2% <100.0%> (ø)
Ical.Net/Evaluation/RecurringEvaluator.cs	95.8% <100.0%> (ø)
Ical.Net/NodaTimeExtensions.cs	100.0% <100.0%> (ø)
....Net/Serialization/DataTypes/DateTimeSerializer.cs	87.8% <100.0%> (ø)
Ical.Net/CollectionExtensions.cs	66.7% <0.0%> (ø)
... and 7 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]

Remove Hour/Minute/Second, and Year/Month/Day

If I remember correctly, this what I mentioned some time ago, and the reply was NodaTime supply these methods too.
So it would simplify the API on one side, but the simple properties don't harm on the other side. Take your choice.

[maknapp]

This latest commit d887305 removes the constructors with separate date and time in favor of just DateTime or LocalDateTime. It also changes some of the constructors to From* and moves them next to the associated To* to match #934 (comment). Thoughts?

[axunonb]

Thoughts

Few things come to my mind:

• ZonedDateTime/Instant got factories, while CalDateTime(LocalDateTime, string? tzId) stays as a public constructor. By intension?
• Generally: If we have a factory, there is no public constructor doing the same thing, right?
• public static CalDateTime Now => new(DateTime.Now); DateTime.Now has DateTimeKind.Local. Would using explicit floating be more clear: public static CalDateTime Now => new(LocalDateTime.FromDateTime(DateTime.Now)); or alike?

[maknapp]

* ZonedDateTime/Instant got factories, while CalDateTime(LocalDateTime, string? tzId) stays as a public constructor. By intension?

CalDateTime stores LocalDate and LocalTime values, so the LocalDateTime and LocalDate constructors felt better to me as constructors since they copy values (mostly) directly. Instant "converts" to UTC and ZonedDateTime drops the offset.

That being said, I'm on the fence about being split like this - should probably be all or nothing. All types beyond int constructors being factories would be explicit and easy to read, and would allow for adding more later. Using constructors with only required factory methods (FromDateTimeDate and FromDateTime) would be easier to use. Do you have an opinion on which way to go?

* Generally: If we have a factory, there is no public constructor doing the same thing, right?

Yes.

* public static CalDateTime Now => new(DateTime.Now); DateTime.Now has DateTimeKind.Local. Would using explicit floating be more clear: public static CalDateTime Now => new(LocalDateTime.FromDateTime(DateTime.Now)); or alike?

Yes.

[maknapp]

CalDateTime should only represent a calendar date/time in UTC and not an Instant, so I removed the To/From for Instant.

I tried to remove all ToZonedDateTime methods and rename AsZonedOrDefault to ToZonedDateTime but I ran into two issues:

1. There are many places (mostly in tests) where it is known that there is a time zone, so being required to give a default was annoying. Having an automatic default of UTC seemed wrong, so I kept a ToZonedDateTime() that throws when there is no time zone.
2. I changed AsZonedOrDefault to ToZonedOrDefault. ToZonedDateTimeWithDefault is pretty long, and using *OrDefault is more similar to things like FirstOrDefault. This way both start with ToZoned* to easily see both options in autocomplete.

[axunonb]

ToZonedDateTimeWithDefault is pretty long, and using *OrDefault is more similar to things like FirstOrDefault. This way both start with ToZoned* to easily see both options in autocomplete.

Yes, the sound like a good way to go.

[axunonb]

This produces a floating CalDateTime, although it looks like current moment.
Should we remove CalDateTime.Now altogether?

[maknapp]

Since users have the option of using NodaTime entirely, I think it is fine leaving this as-is for those who want to keep using DateTime.

[axunonb]

The previous version threw on DateTimeKind.Local. Now we silently get a floating CalDateTime.
If we go this way, XML doc should at minimum call this out explicitly, even if throwing is considered too strict.

[maknapp]

I changed the constructor to FromDateTime and changed the summary to say "Converts a DateTime of any kind to..." (similar to NodaTime.FromDateTime summary). Is that good?

[sonarqubecloud]

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

[maknapp]

I think this is ready to merge now. We can see how these changes feel and remove/change more later. I removed the time properties to force users to check for null themselves. While adding tests I noticed that GetHashCode did not match Equals so I changed that too.