dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.17k stars 4.72k forks source link

[API Proposal]: method overload `DateTimeOffset.Equals(DateTimeOffset other, TimeSpan margin)` #101393

Open alexandrehtrb opened 6 months ago

alexandrehtrb commented 6 months ago

Background and motivation

Currently, it is difficult in .NET to compare equality of DateTime(Offset) when parts need to be ignored, such as milliseconds and seconds. Check this Stack Overflow question.

API Proposal

Same for TimeSpan, DateTime and DateTimeOffset. EqualsExact would also have this overload, but considering TimeZoneOffset / DateTimeKind too.

namespace System;

public readonly struct DateTimeOffset
{
    public bool Equals(DateTimeOffset other, TimeSpan margin);
}

API Usage

var dt = DateTime.Parse("Wed, 21 Oct 2015 07:28:00 GMT");
TimeSpan margin1ms = TimeSpan.FromMilliseconds(1.0),
         margin1s = TimeSpan.FromSeconds(1.0),
         margin1min = TimeSpan.FromMinutes(1.0);

if (DateTime.Now.Equals(dt, margin1ms))
{
  Console.WriteLine("Same date and time, up to milliseconds precision");
}
else if (DateTime.Now.Equals(dt, margin1s))
{
  Console.WriteLine("Same date and time, up to seconds precision");
}
else if (DateTime.Now.Equals(dt, margin1min))
{
  Console.WriteLine("Same date and time, up to minutes precision");
}

Alternative Designs

Open for suggestions.

Risks

I don't see any, at first sight.

dotnet-policy-service[bot] commented 6 months ago

Tagging subscribers to this area: @dotnet/area-system-datetime See info in area-owners.md if you want to be subscribed.

MichalPetryka commented 6 months ago

I wonder if exposing some DateTimeOffset.Truncate method for this instead would make more sense.

Clockwork-Muse commented 6 months ago

...As a point of personal preference, date/time values should generally be considered equivalent to floating point (yes, it's generally exactly representable, but due to the measurement nature you can still end up with odd values). This tends to make me want to encourage one of two general behaviors:

alexandrehtrb commented 4 months ago

checking whether a value is in that range

I think this is already covered by today, isn't it? Like:

var dt = DateTime.Parse("Wed, 21 Oct 2015 07:28:00 GMT");
bool isToday = DateTime.Today <= dt && dt < DateTime.Today.AddDays(1.0);

Equals with an "ulp"

Is an "ulp" an error margin? Then the API would be something like:

public readonly struct DateTimeOffset
{
    public bool Equals(DateTimeOffset other, TimeSpan margin);
}

I like this latter option too.

Clockwork-Muse commented 4 months ago

Yes... but I'm hesitant to use a type that would exclude years or months, just on general principle.

alexandrehtrb commented 4 months ago

Yes... but I'm hesitant to use a type that would exclude years or months, just on general principle.

I think that using a DateTimeComparison enum would be a better option for most cases. If we use adopt a method with TimeSpan margin, the programmer would have to allocate this margin, which is more work for him / her; but if we adopt an enum, then no need for that. The enum also would solve the problem you mentioned - considering up to years, months, etc.

Clockwork-Muse commented 4 months ago

The problem is that the proposed enum is generally too coarse. For instance, it doesn't allow for comparisons at a 10ms boundary... and that assumes that, especially at smaller measurements, truncation is the desired behavior. For instance, the historical "step" of consumer computer clocks was ~15ms, and there wasn't much control over when the value was for. Certain historical file times are limited to 2s precision. Things like that.

julealgon commented 4 months ago

What about providing both overloads @Clockwork-Muse ? The enum-based one for simpler, more common scenarios, and the TimeSpan one for more advanced, specific ones?

Clockwork-Muse commented 4 months ago

The problem is that the "more advanced" one has different behavior.

The proposed one truncates and buckets to the specified (very coarse) precision. The "more advanced" one actually does an absolute difference comparison (since the other value may be on the other side of the boundary).

Again, if the truncating/bucketing behavior is what you want, then from an application/domain design perspective probably you should be starting from the bucket (Eg, as if from an SQL calendar table) and stride the ranges. Note that in the majority of such cases, you're going to have the bounds anyways (you're not going to be comparing two arbitrary precision values).

If you want "close to" values for arbitrary precision values, then you need an "error range", which means you need an absolute difference comparison, which this does not provide.

the programmer would have to allocate this margin, which is more work for him / her; but if we adopt an enum, then no need for that.

This is mostly a non-issue. 1) The type is a struct, so there are no literal allocations (although there are constructor costs). 2) At scale, the value can be allocated outside a loop or as a readonly static value, so the call will look effectively identical, eg;

someValue.Equals(other, TimeDifference.Within10Milliseconds);
alexandrehtrb commented 4 months ago

@Clockwork-Muse , what do you think of the proposal below? @julealgon said we can have both overloads.

namespace System;

public enum DateTimeComparison
{
  UpToDate,
  UpToHour,
  UpToMinutes,
  UpToSeconds,
  UpToMilliseconds,
  UpToMicroseconds,
  UpToNanoseconds
}

public readonly struct DateTimeOffset
{
    public bool Equals(DateTimeOffset other, DateTimeComparison comparisonType)
    {
        var thisTruncated = this.Truncate(comparisonType);
        var otherTruncated = other.Truncate(comparisonType);
        return thisTruncated == otherTruncated;
    }

    public bool Equals(DateTimeOffset other, TimeSpan margin)
    {
        var diffTicks = Math.Abs((this - other).Ticks);
        var marginTicks = Math.Abs(margin.Ticks);
        return diffTicks <= marginTicks;
    }
}
Clockwork-Muse commented 4 months ago

Potential benefits of a Truncate method aside (although I might prefer something that takes TimeSpan instead, since the enum might otherwise be too coarse), I feel like I'm generally against a truncation comparison as an on-type equality comparison.

I think I would want to recommend some form of:

This is because a truncation comparison is essentially a "lossy" comparison (As opposed to the margin/ulp comparison)

alexandrehtrb commented 4 months ago

My opinion as a developer is that having a one-liner, straight-to-the-point method, is better for user experience, as I believe that in most cases this truncation would be used only for later comparison.

We can add a <summary> on the overload to inform users that it performs a lossy comparison.

alexandrehtrb commented 1 month ago

@Clockwork-Muse , how about we go for both methods?

public bool Equals(DateTimeOffset other, TimeSpan margin);

public bool Equals(DateTimeOffset other, DateTimeComparison comparisonType);

The first one uses a margin. This is the equality check that xunit uses for assertion.

I still believe that the second one is a good option, so the users won't need to declare a private static readonly TimeSpan timeMargin = TimeSpan.FromSeconds(1); for equality checks. They can simply use an enum value.

Clockwork-Muse commented 1 month ago

I still believe that the second one is a good option, so the users won't need to declare a private static readonly TimeSpan timeMargin = TimeSpan.FromSeconds(1); for equality checks. They can simply use an enum value.

  1. As I pointed out previously, the enum-based check isn't performing an ulp-like/margin comparison, which means the two methods have different behavior in the first place, which is potentially confusing.
  2. The primary thing I'm trying to get across is that not having the truncation comparison method is a deliberate design choice to force programmers to think about their domain/algorithm. Requiring the declaration and bucketing/striding behavior is in service to that. It's meant to be in service to "make the right thing easy, and the wrong thing hard".

The fundamental problem is that truncation comparisons like the enum-based method would produce are almost always "wrong" from a domain design/algorithm/data perspective. In almost all real cases I can think of, the programmer should be starting from a bucket/striding perspective, as that will most closely match what it is they're actually trying to accomplish. Especially with the way the enum is designed. If you can provide me a reasonable counter example I'm willing to be proven wrong, however I imagine most of the cases you're thinking of a probably better served by the ulp comparison.

alexandrehtrb commented 1 month ago

I think I understand what you mean.

var dt1 = DateTimeOffset.Parse("2024-07-09T17:34:22.800000-03:00");
var dt2 = DateTimeOffset.Parse("2024-07-09T17:34:23.100000-03:00");

bool truncateEquals = dt1.Equals(dt2, DateTimeComparison.UpToSeconds); // false
bool marginEquals = dt1.Equals(dt2, TimeSpan.FromSeconds(1)); // true

// we care for a 1s margin and there is a 300ms diff,
// but the two methods return different values.

Considering that, I agree that the margin method is better. But, how about adding a TimeSpan.OneSecond constant? So people could simply type dt1.Equals(dt2, TimeSpan.OneSecond). A constant for 1 minute would be good too.

Clockwork-Muse commented 1 month ago
  1. I would recommend updating the proposal at the top with the ulp-based method. (There's maybe a separate conversation about a new relative-offset type, which may make #72064 more attractive)
  2. I don't know that I'd recommend additional TimeSpan constants, even for such a "common" one as One (the existence of Zero is a different special case). Most of them are going to be too application-specific, and they're trivial to create when needed. And in most cases, should be coming from parameters or application config anyways.