Version 5

[axunonb]

@minichma New discussion about v5

[axunonb]

There is a new label to flag issues or posts with a green "version 5"

[axunonb]

@minichma Created a project https://github.com/orgs/ical-org/projects/3/views/1 and sent an invitation for team "Maintainers" to you. You have access, as soon as you'll accept. I already put first project items there.

[minichma]

@axunonb I don't think I received an invitation.

[axunonb]

Just re-sent now. Currently it's displayed as "pending invitation"

[minichma]

now it worked, thanks

[axunonb]

@minichma
Thanks for your comments to CalDateTime.
The aim of the PR was mainly to fix issues while keep compatibility. Really not sure, whether this is a good way go...
Would your preferred alternative be to leave v4 as is, and (more or less) rewrite v5?

Trying to summarize how I understood the remarks:

Use of NodaTime for Date and Time Representation

Suggestion: Use NodaTime's LocalDateTime, DateOnly, or TimeOnly instead of System.DateTime.

Reasoning: NodaTime provides a more precise and unambiguous representation of date and time values. System.DateTime can be ambiguous due to the Kind property, which can lead to confusion about whether the time is local, UTC, or unspecified.

Action: Consider refactoring CalDateTime to use NodaTime types. This will make the class more robust and less prone to errors related to time zone handling.

Timing: In #638

Immutability of CalDateTime

Suggestion: Make CalDateTime immutable.

Reasoning: Immutable types are generally easier to reason about and less prone to bugs, especially in multi-threaded environments. They ensure that once an instance is created, its state cannot be changed.

Action: Refactor CalDateTime to be immutable. This might involve removing setters from properties and using constructor parameters to set initial values.

Timing: Together with another breaking change.

Necessity of IDateTime Interface

Suggestion: Evaluate the necessity of the IDateTime interface.

Reasoning: If CalDateTime is the only implementation of IDateTime in ical.net, the interface might be unnecessary and could introduce additional complexity without providing significant benefits.

Action: Review the usage of IDateTime. If it is not providing clear benefits, consider removing it and using CalDateTime directly.

Timing: Together with another breaking change. Until then, mark HasTime and HasDate setters as obsolete.

AssociatedObject Property

Suggestion: Re-evaluate the need for the AssociatedObject property in CalDateTime.

Reasoning: The AssociatedObject property might be mixing concerns by combining the representation of a date-time value with operations related to the calendar model. This can lead to confusion and make the class harder to maintain.

Action: Consider separating the concerns of date-time representation and calendar operations. This might involve creating separate classes or methods for operations that act on CalDateTime instances.

Timing: Mark as obsolete in #638

Separation of Model and Operations

Suggestion: Improve the separation between the model (e.g., CalDateTime) and operations performed on it.

Reasoning: Clear separation between data representation and operations can make the code more maintainable and easier to understand. It also helps in keeping the data model immutable and performing calculations just in time.

Action: Review the design of CalDateTime and related classes. Consider creating separate classes or methods for operations that act on CalDateTime instances, ensuring that the data model remains unchanged.

Timing: Undefined.

[minichma]

@axunonb Sorry for being verbose on #638. For whatever reason, probably due to previous discussions, I was thinking that the code was intended for v5, so compatibility wouldn't be an issue. Anyhow, your summary of my comments is quite accurate. All those are just thoughts that came to my mind when reading and working with the code. As CalDateTime is so fundamental, I was struggling with it (and related types like Period) quite a bit. The proposed modifications would have helped in understanding (e.g. immutability, less 'magic'). Looking at the past and current issues, they might also have helped in avoiding some of them (e.g. by avoiding DateTimeKind.Local). That said, it's clear that the changes would come with quite some work and also require consumers of the lib to apply significant adjustments to their own code, which they may or may not be ready for. However, now, after the project was in a sleeping state for so long, would be the right time to do that. I even think that, as people have been living with the v4 issues for so long, and the maintainer's time is sparse, its probably more efficient to invest time into v5 rather than on v4. In some cases it might not even be easily possible to fix v4 issues while maintaining backwards-compatibility. At the same time I must say, that I'm only commenting from the side line, as I probably won't have the time to do significant contributions (sorry for repeating myself). And the comments might be right or wrong, they're just my opinion. It's really on you to decide which way to go.

[axunonb]

@minichma As we both don't have the capacity to come up with a "brand new v5" the way to go could be

• continue improving the current code base as much as possible. If fixing means a breaking change, we publish a new major version, so we can expect more major versions in the near future.
• increase code coverage

Do you see a chance to complete the work about recurrence that you already started?

[minichma]

@axunonb Agree.

If fixing means a breaking change, we publish a new major version, so we can expect more major versions in the near future.

Sure. We should keep in mind though, that new major versions will probably also require ongoing maintenance, so it could be a viable option to introduce a v5 preview/beta/whatever and keep working on that for a while, until we feel the most important changes have been made.

Do you see a chance to complete the work about recurrence that you already started?

Yo mean aside from #598 (that one would b ready and is basically waiting for v5). Sure, I should be able to have a look at a few more recurrence-related issues.

[axunonb]

DATE value type in CalDateTime

@minichma
Currently I'm working on #646.
Reading RFC 5545 Section 3.3.4, where the the DATE value type is defined, it says that the DATE value type represents a calendar date without any time or time zone information.
My interpretation for ical.net is, that initialization with DateOnly should never include a timezone
Besides

public CalDateTime(DateOnly date, TimeOnly? time, string? tzId = null)
{
    Initialize(date, time, tzId);
}

with the time argument NOT nullable we need an overload

public CalDateTime(DateOnly date)
{
    Initialize(date, null, null);
}

which must be used for the date-only case. Am I correct?

[axunonb]

@minichma
Maybe discuss the PR comment here:
My understanding of comparing floating to zoned date/times is as shown in the code above: When comparing a floating date/time to a zoned date/time, the comparison should be based on the values, ignoring the time zone information.
That's the reason for

if (left.IsFloating || right.IsFloating)
{
    return left.Value.Equals(right.Value)
           && left.HasTime == right.HasTime;
}

It's also the reason why this should work for floating date/times - just add the timezone:

    public IDateTime ToTimeZone(string otherTzId)
    {
        if (IsFloating) return new CalDateTime(_dateOnly, _timeOnly, otherTzId);
        // ... non-floating code
    }

Is there sth. I'm missing?

[minichma]

Well, if one has a tzId and one hasn't, then I'd say, they simply aren't equal. One aspect is that equality operators should be transitive. I.e. if new CalDateTime("20241204T000000", "Europe/Vienna").Equals(new CalDateTime("20241204T000000", null) and new CalDateTime("20241204T000000", null).Equals(new CalDateTime("20241204T000000", "America/New_York") then transitivity would require new CalDateTime("20241204T000000", "Europe/Vienna").Equals(new CalDateTime("20241204T000000", "America/New_York") which obviously isn't the case. For that reason it can also easily be a source of bugs.

[minichma]

Here's another example. The following test would fail, if Equals is not transitive (and GetHashCode() is properly implemented, which actually isn't possible if Equals isn't transitive though):

    [Test]
    public void Test()
    {
        var seq1 =
            new CalDateTime[]
            {
                new CalDateTime("20241204T000000", "Europe/Vienna"),
                new CalDateTime("20241204T000000", null),
                new CalDateTime("20241204T000000", "America/New_York"),
            }.Distinct();

        var seq2 =
            new CalDateTime[]
            {
                new CalDateTime("20241204T000000", "Europe/Vienna"),
                new CalDateTime("20241204T000000", "America/New_York"),
                new CalDateTime("20241204T000000", null),
            }.Distinct();

        Assert.That(seq1.Count(), Is.EqualTo(seq2.Count()));
    }

[axunonb]

equality operators should be transitive

Now hopefully I get your point, thanks for being patient.
It's about transitive equality on one side and equality in a certain context like zoned vs. floating date/times, which is not the same thing For the latter latter we need another method to compare, and it cannot be an equality operator.
Correct?

[axunonb]

Committed to PR in cd878f9

[axunonb]

CalDateTime.AsDateTimeOffset

@minichma
As far as I see, in #658 this is the only place left, where DateTimeKind.Local comes into play.
The system's local timezone may or may not be the right context for the user.
Would it be good way to implement a setting for the user's local timezone? If so, where should this setting go to?

/// <inheritdoc />
/// <remarks>
/// 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();

[minichma]

As far as I see, in #658 this is the only place left, where DateTimeKind.Local comes into play.

Nice! :-)

Would it be good way to implement a setting for the user's local timezone?

I think the cleanest way would be, not to make the local timezone a special case at all. Rather than having the TZ configured and having special code to deal with it, we could provide methods that can deal with any timezone (which probably exists in most cases) and make it easy for the user to specify their own one, e.g. by introducing something like

public static class LocalTimeZone {

    public static string TzId => DateUtils.LocalDateTimeZone.Id; // or the like

    public CalDateTime Now => new CalDateTime(DateTime.Now, TzId);

   ...
}

Users could then easily pass LocalTimeZone.TzId whereever a tz is required and they want to use the local one. This way we don't introduce additional state that we have to maintain.

[axunonb]

Absolutely! And remove DateTimeOffset, right?

[minichma]

Not sure what it is used for. I never used it. Maybe just remove it and see if someone complains. Or alternatively throw an exception if called on a CalDateTime with floating time.