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

How should Temporal arithmetic handle duration units smaller than the receiver's smallest unit? #2044

Closed
justingrant opened this issue Feb 7, 2022 · 2 comments
Assignees
Labels
documentation Additions to documentation
Milestone

Comments

@justingrant
Copy link
Collaborator

justingrant commented Feb 7, 2022

While reviewing #2003 and looking at implementation concerns raised in #1863, I looked more closely at the PlainYearMonth add/subtract algorithm in the spec and noticed interesting behavior: when adding (or subtracting a negative duration), we treat the receiver as if it were the first nanosecond of the month, but when subtracting we treat it as the last nanosecond of the month. The same behavior seems to be present in PlainDate add/subtract, where subtracting one hour from a PlainDate yields the same value because we treat the receiver as if was the last nanosecond of the day.

Is this behavior what callers will expect? Specifically:

  • AFAICT, in all other Temporal contexts we treat a PlainDate as if it were the first nanosecond of the day, and treat a PlainYearMonth as the first nanosecond of the month. Why is this case different? For example, in Calendar-ignoring compare behaves unexpectedly for non-ISO calendar PlainYearMonth comparisons #1846 we decided that when comparing PlainYearMonth instances across calendars, we'd treat a PlainYearMonth as the first day of the calendar month.
  • When combining addition with converting PlainDate to PlainDateTime (or PlainYearMonth to PlainDate or PlainDateTime), will it be expected that order-of-operations should cause different results? EDIT: I updated this example to be clearer
/** add a duration to a date, and then return a PlainDateTime for noon on the resulting day */
function noonOnAddedDate1(plainDate, duration) {
  const pdt = plainDate.toPlainDateTime();
  const added = pdt.add(duration);
  const result = added.withPlainTime('12:00');
  return result;
}
/** same as above, but will return different result for negative durations with nonzero time units */
function noonOnAddedDate2(plainDate, duration) {
  const added = plainDate.add(duration);
  const result = added.toPlainDateTime('12:00');
  return result;
}
date = Temporal.PlainDate.from('2020-01-01');
duration = Temporal.Duration.from('-PT12H');
noonOnAddedDate1(date, duration);
// => 2019-12-31T12:00:00
noonOnAddedDate2(date, duration);
// => 2020-01-01T12:00:00

Anyway, seems like we have three options:

  1. Leave current behavior as-is, and document it
  2. Change add/subtract to be consistent with other part of temporal by treating PlainDate as the first nanosecond of the day and PlainYearMonth as the first nanosecond of the month.
  3. Throw if the user calls PlainDate add/subtract with non-zero hours (or smaller units) or calls PlainYearMonth with non-zero days (or smaller units).

My opinion is that (2) or (3) will be easier for callers to predict and understand. I'm not sure any developer will expect (1) unless they read the spec or (when we update them) the docs.

@ptomato
Copy link
Collaborator

ptomato commented Feb 8, 2022

The previous discussion on this is in #324

The reason I think (1) is not so weird, is because of the semantics we came up with in that thread: when adding duration units smaller than the receiver's smallest unit, balance them up to the smallest valid unit for that type, and discard any remainder. That's equivalent to the semantics that the spec describes for PlainYearMonth (balance up to days and treat the date as the last day of the month when subtracting), only it saves some observable calendar method calls, but sounds a lot more intuitive.

A counter to (2) would be that I'd find this pretty unexpected:

const date = Temporal.Now.plainDateISO();
date.add({ hours: 12 })  // same date
date.subtract({ hours: 12 }) // different date?!

I think I probably left this out of the documentation for brevity, but I personally wouldn't mind if it were added now. I'd make different choices nowadays than I did 2 years ago on what to include since we're in a different stage of the proposal now.

Aside from the above...

When combining addition with converting PlainDate to PlainDateTime (or PlainYearMonth to PlainDate or PlainDateTime), will it be expected that order-of-operations should cause different results?

I think I disagree with you that this is unexpected. I wouldn't expect these to be order-independent. I thought your original example (now edited out) was actually more illustrative of this. IIRC it was

  • date.add({hours: 1}).toPlainDateTime() vs.
  • date.toPlainDateTime().add({hours: 1})

If these had to produce the same result, that would mean that PlainDate would have to have hidden state where it kept track of the 1 hour that had been added before it was converted to PlainDateTime.

@ptomato
Copy link
Collaborator

ptomato commented May 10, 2022

Given that this is a decision that we already made in #324, and no new information has come to light, I'm strongly in favour of (1): leave the current behaviour as is, and document it.

@ptomato ptomato added the documentation Additions to documentation label May 10, 2022
@ptomato ptomato added this to the Post Stage 4 milestone Dec 8, 2022
@ptomato ptomato self-assigned this Mar 26, 2024
ptomato added a commit that referenced this issue Mar 26, 2024
@Ms2ger Ms2ger closed this as completed in 1e996e5 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants