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

Why no Duration.prototype.compare ? #608

Closed
justingrant opened this issue May 22, 2020 · 7 comments
Closed

Why no Duration.prototype.compare ? #608

justingrant opened this issue May 22, 2020 · 7 comments

Comments

@justingrant
Copy link
Collaborator

Duration doesn't have a compare method. Is this intentional?

@ptomato
Copy link
Collaborator

ptomato commented May 22, 2020

It is intentional, because for some units in Temporal.Duration, you can't tell how long the duration actually is, without a start or end date. For example, which is longer: Temporal.Duration.from({ months: 2 }) or Temporal.Duration.from({ days: 60 })? If you start counting from 2019-01-01, then the latter is longer, but if you start from 2020-07-01, then the former is longer.

The way to compare durations would be to consider them relative to a start date: Temporal.DateTime.compare(start.plus(duration1), start.plus(duration2)) (or Date or Absolute as appropriate)

@gibson042
Copy link
Collaborator

gibson042 commented May 22, 2020

I don't think that solves the problem.

const P2M = Temporal.Duration.from("P2M");
const P60D = Temporal.Duration.from("P60D");
function compareDurations(a, b, start) {
  return Temporal.DateTime.compare(start.plus(a), start.plus(b))
}
compareDurations(P2M, P60D, Temporal.DateTime.from("2019-02-01T00:00")); // → -1
compareDurations(P2M, P60D, Temporal.DateTime.from("2019-07-01T00:00")); // → 1
compareDurations(P2M, P60D, Temporal.DateTime.from("2020-02-01T00:00")); // → 0

I'm also surprised by the absence of Temporal.Duration.compare, and would rather see exceptions from incomparable values¹ than an no easy way to check comparable ones.

¹ i.e., rejecting input with nonzero values on both sides of the months/days boundary, and rejecting or accepting input with nonzero values on both sides of the days/hours boundary consistently with the rest of Temporal

@justingrant
Copy link
Collaborator Author

justingrant commented May 23, 2020

@gibson042 I'm also surprised by the absence of Temporal.Duration.compare, and would rather see exceptions from incomparable values¹ than an easy way to check comparable ones.

I think this is an interesting proposal worth investigating. My main concern would be that exceptions might only show up at runtime, as opposed imposing limits in the types themselves (e.g. no days property).

BTW, after reviewing the entire Temporal API surface while building TypeScript types, the only type that feels like it has fundamental problems (i.e. not just naming or other minor issues) is Duration. I think you're right that the root cause is the challenges converting days to months (needs a starting date) and hours to days (needs a starting date and time zone because DST).

Here's a summary:

  • No negative durations? #558 - No negative durations, unlike every single other duration API I could find across 10+ platforms
  • How to round/ceil/floor? #560 - awkward ergonomics for round/ceil/floor
  • No compare
  • Math doesn't work from days->months (for same reasons @ptomato lists above) but does work for hours->days, which exposes potential for DST issues
  • Total methods for Duration? #584 Awkward ergonomics & bad discoverability for calculating total values (e.g. total number of days, seconds, etc. in a duration), esp. relative to APIs like .NET's which have easily-discoverable .TotalXxx() methods.
  • Absolute accepts and emits days in durations which can cause DST problems

I've been thinking over ways to address these problems in a holistic way, but haven't come up with any solutions yet. Probably a good thing to discuss in real time. Some questions to consider:

  • Should Duration be split into Difference type (which has all units but no arithmetic & no balancing) and Duration (which is just a number of nanoseconds-- and maybe a TZ offset delta) plus some convenience methods to calculate hours and smaller units and totals)?
  • Should there be a DateTimeDuration (clock time) and an AbsoluteDuration (nanoseconds) to match the rest of the object model? The concepts of Difference vs. Duration in the bullet point above are essentially these types.
  • Should Absolute's plus, minus, and difference methods accept or emit days, given the DST issues involved?
  • Should there be a TimeDifference (with math, balancing, and compare but no days/months/years) and a DateDifference` (with compare but no math)?
  • Should Duration be replaced with a type that records a start and end, to enable math and comparison on all units?
  • Can Duration balancing be simplified so it's easier to understand and use?
  • How can Duration be made forward-compatible if we want to add a ZonedAbsolute type at some point in the future? In particular, should Duration have timeZoneOffsetDeltaNanoseconds which would enable converting a nanoseconds duration into a clock time delta while accounting for a DST transition during the period?

@rbuckton
Copy link

rbuckton commented Jun 2, 2020

Should there be a TimeDifference (with math, balancing, and compare but no days/months/years) and a DateDifference (with compare but no math)?

IIRC, the terms for these are, commonly:

  • Accurate duration: An accurate representation of time in SI units regardless of calendar (i.e., hours, minutes, seconds, milliseconds, microseconds, and nanoseconds).
  • Nominal duration: A representation of time dependent on a calendar (i.e., years, months, days).

That said, I'm more commonly familiar with .NET's TimeSpan class, which always represents an accurate duration and as such is always balanced and is comparable.

@ptomato
Copy link
Collaborator

ptomato commented Aug 12, 2020

Now that we've thoroughly revamped durations already, it might be time to tackle this one! What about:

  1. Add an equals() method and a compare() static method. If the durations contain nonzero years, months, weeks, or days, then compare() throws. Unless the durations are equal, maybe?
  2. Add an equals() method to Temporal.Duration but not a compare() static method

My weakly-held preference is for 2; compare() sometimes throwing seems dangerous to me, and so far I haven't had any need to sort durations, but I'd change my mind if that changes.

It seems like Temporal.Duration.prototype.equals() ought not to be controversial though!

@justingrant
Copy link
Collaborator Author

It's too soon for this, IMHO. I think the best sequencing will be to resolve the duration rounding decisions first (the discussion that we postponed from the 2020-07-31 marathon meeting after we couldn't resolve it after over an hour!), and then circle back to downstream questions like how should equals and compare be handled.

It's possible that a parameter-less equals that requires strict equality (every unit must match, so PT120S and PT2M would be false) would be OK because it doesn't depend on balancing, but IMHO it's probably more efficient for everyone to just put this on ice for a week or two so we'll have time to first resolve the bigger rounding/balancing questions.

@justingrant
Copy link
Collaborator Author

Meeting 2020-09-11: We'll merge this into #856. Please continue discussion over there.

Also, tentative conclusion (@pipobscure had the last word) was that we would not offer an equals method (only compare) because the meaning of equals is too ambiguous, e.g. is PT60S equal to PT1M? Better to force users to use compare with the relativeTo option (see #856).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants