Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
alexandrehtrb opened this issue Apr 22, 2024 · 16 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.DateTime
Milestone

Comments

@alexandrehtrb
Copy link

alexandrehtrb commented Apr 22, 2024

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.

@alexandrehtrb alexandrehtrb added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 22, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 22, 2024
Copy link
Contributor

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

@tarekgh tarekgh added this to the Future milestone Apr 22, 2024
@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Apr 22, 2024
@MichalPetryka
Copy link
Contributor

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

@Clockwork-Muse
Copy link
Contributor

...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:

  • Explicit range comparisons and striding, especially for bucketing. Instead of performing an equality comparison as here, in many cases it will be preferrable to construct ranges (of the form lower-bound inclusive, upper-bound exclusive), and checking whether a value is in that range. Especially at larger units this is generally how the domain is actually being used (eg, bucketing for an hourly report) - and note that it works correctly for all units, including years.
  • Equals with an "ulp", as how floats are compared. In some small number of cases (possibly only when dealing with smaller units), it may be more beneficial to compare with an ulp; consider that at smaller units especially there is a high likelihood the values you're interested in may be on the "wrong side" of the unit boundary.

@alexandrehtrb
Copy link
Author

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
Copy link
Contributor

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

@alexandrehtrb
Copy link
Author

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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:

  • Perform the truncation outside the equals, then check the equality on the resulting value(s).
  • Implement EqualityComparer.

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

@alexandrehtrb
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

  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 [API Proposal]: BCL Type to Represent ISO8601 Durations #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.

@alexandrehtrb alexandrehtrb changed the title [API Proposal]: method overload DateTimeOffset.Equals(DateTimeOffset other, DateTimeComparison comparisonType) [API Proposal]: method overload DateTimeOffset.Equals(DateTimeOffset other, TimeSpan margin) Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.DateTime
Projects
None yet
Development

No branches or pull requests

5 participants