`CalDateTime.AddDuration` vs. `CalDateTime.AddDay`

[axunonb]

Describe the bug
CalDateTime.Add(Duration.FromDays(1) and CalDateTime.AddDay(1) do not return the same result.

To Reproduce

[Test]
public void AddOneDay()
{
    // Start Time: March 29, 2025, at 02:30 CET.
    // DST transition on March 30, 2025 at 02:00
    var dt = new CalDateTime(2025, 3, 29, 2, 30, 0, tzId: "Europe/Berlin");

    var resultFromDuration = dt.Add(Duration.FromDays(1));
    var resultAddDays = dt.AddDays(1);

    Assert.That(resultFromDuration.Value, Is.EqualTo(resultAddDays.Value));
}

Same with other arithmetic operations with nominal values.

Expected behavior
Results should be equal.

Environment:

• OS: Windows 11
• .NET version: NET8.0
• ical.net version: 5.1.0

[minichma]

Good catch. In the Add(Duration) overload we handle the case that the result is non-existent but in the AddDays() method we don't. See here:

ical.net/Ical.Net/DataTypes/CalDateTime.cs

Lines 515 to 517 in 35b300d

	if (TzId is not null)
	// Convert to the original timezone even if already set to ensure we're not in a non-existing time.
	newDateTime = newDateTime.ToTimeZone(TzId);

Both are useful at times.

BTW, it seems that the method returns a UTC instance if adding an exact value to floating time. Probably wrong too.

ical.net/Ical.Net/DataTypes/CalDateTime.cs

Lines 512 to 513 in 35b300d

	if (dt.exactPart is not null)
	newDateTime = new CalDateTime(newDateTime.AsUtc.Add(dt.exactPart.Value), UtcTzId, HasTime);

[axunonb]

Both are useful at times

Some methods return zoned results, others don't. For the public API this is heavy stuff, isn't it? If someone needs it CalDateTime.Value.AddDays(...) is more transparent.

Probably wrong too

Indeed.

[axunonb]

@minichma How do you think we could proceed with the arithmetic operations in general, and with this issue in particular? Not sure how to interpret the comment on the latest discussion

[minichma]

@axunonb Regarding this particular issue I guess there is no best option. We could

1. Adjust AddDays() to correspond to Add(Duration)
   • pro: consistent behavior
   • contra: as today's behavior cannot be clearly identified as bug, this would have to be considered a breaking change.
2. Other way round
   • pro: none
3. Mark AddDays() as deprecated and leave it as is. Maybe introduce dedicated methods for representing the current behavior but with clear naming.
   • pro: reasonably clean solution, no breaking change
   • contra: for v5 its quite early to introduce braking changes already
4. Ignore the issue as it 'works as implemented' and add corresponding documentation
   • pro: easy, no breaking change
   • contra: somewhat counter-intuitive

IMHO any (except 2) would be feasible. I tend to prefer 1 as one could argue, the current behavior is a bug and chances are low that this will cause issues anywhere.

Regarding the future of arithmetic operations I already mentioned in the past that I think it would be helpful to make NodaTime a first class dependency and leave all the arithmetics to them. I.e. implement clear translation functionality between both and then do all calculations to Noda. This would imply that GetOccurrences() et all would all be working with Noda types rather than with CalDateTime. Only those operations that are specific to RFC 5545 should be implemented in Ical.Net. The most notable challenge that I see is that in NodaTime there is no class representing any of date-time with tz, floating date-time as well as date-only in a single type. Probably we'd have to convert all of those to ZonedDateTime one way or the other.

Anyhow, not a simple task.