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

Negative durations vs. order of operations #913

Closed
justingrant opened this issue Sep 16, 2020 · 9 comments
Closed

Negative durations vs. order of operations #913

justingrant opened this issue Sep 16, 2020 · 9 comments
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

We previously decided that the order of operations in Temporal would be largest-units-first for addition and smallest-units-first for subtraction. The goal was for a.plus(x).minus(x) to always return the same value.

My assumption that now with negative durations, it should work like this:

  1. adding a positive duration - largest first (unchanged from before)
  2. adding a negative duration - smallest first (same as subtracting before)
  3. subtracting a positive duration - smallest first (unchanged from before)
  4. subtracting a negative duration - largest first (same as adding a positive duration)

Is this correct?

@ptomato ptomato added this to the Stable proposal milestone Sep 17, 2020
@ptomato ptomato added documentation Additions to documentation spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! and removed documentation Additions to documentation labels Sep 18, 2020
@justingrant
Copy link
Collaborator Author

OK, now I'm confused. It seems that reversing the order-of-operations doesn't actually enable reversible behavior, at least in some cases. Example:

Temporal.Date.from('2020-01-31').plus({months: 1}).plus({days: 5}).minus({days: 5}).minus({months: 1})
// => 2020-01-29 (order of operations in reverse, but result IS NOT the same as starting point)

Temporal.Date.from('2020-01-31').plus({months: 1}).plus({days: 5}).minus({months: 1}).minus({days: 5})
// => 2020-01-31 (order of operations not reversed, but result IS the same as starting point)

@pipobscure - Was this how you expected it to work? Or was there a different kind of reversibility you had in mind?

@gibson042
Copy link
Collaborator

I think the most significant aspect of this is in the processing within individual operations, e.g. Temporal.Date.from('2020-01-31').plus({months: 1, days: 5}) or Temporal.Date.from('2020-02-29').plus({years: 1, months: 2}) (cf. #58). And AFAICT, that should mean always dealing with the largest units first.

@justingrant
Copy link
Collaborator Author

And AFAICT, that should mean always dealing with the largest units first.

@gibson042 Does you mean that subtraction should also use largest-units-first? (Currently AFAIK the agreed-upon behavior is to reverse the order for subtraction, although I'm not sure how the polyfill and spec are currently implemented.)

@gibson042
Copy link
Collaborator

Essentially, yes. I don't think it makes any sense for Temporal.Date.from('2020-03-05').minus({months: 1, days: 5}) to yield 2020-01-29 or for Temporal.Date.from('2021-04-29').minus({years: 1, months: 2}) to yield 2020-02-28. My expectation would be that every duration field is applied all at once and then the resulting value is normalized ("balanced"?) from largest to smallest (e.g., [2020, 3 - 1, 5 - 5] → [2020, 2, 0] → 2020-01-31 inside the logic of the first example).

@justingrant
Copy link
Collaborator Author

I think this is different behavior from what @pipobscure has argued for in the past. Philipp could you take a look?

@pipobscure
Copy link
Collaborator

pipobscure commented Oct 14, 2020

I consider the examples tha @gibson042 gave edgecases. The relevant thing to me is:

(pseudo code)

let a = DateTime.from(isoA);
let b = DateTime.from(isoB);
let d1 = a.difference(b);
let d2 = b.difference(a);

b.plus(d1).equals(a);
b.minus(d2).equals(a);
a.plus(d2).equals(b);
a.minus(d1).equals(b);

I achieved this by ensuring that difference never generated a duration where a part of the calculation created an invalid intermediate and reversing the order of operations for plus and minus.

So to be clear it requires BOTH inverse order of operations AND difference being smart about how to split units.

And that’s also why I consider the examples edge cases, because it entails manually specifying a bad unit split.

P.S.: the reason for pseudo code is that I’m writing on my phone and therefore have limited reference material 😀

@justingrant
Copy link
Collaborator Author

To clarify: @pipobscure would you expect that your pseudocode should work for all largestUnit values? Or should it only work in cases where units are always the same length but not for varying-length units like months, years, some non-ISO weeks, and ZonedDateTime days across DST transitions?

I tried to validate the pseudocode with real dates but ran into #993 which (assuming it's a legit bug) may prevent validating the pseudocode above in the current polyfill. Any insight you guys have about the expected behavior of #993 would be helpful.

@justingrant
Copy link
Collaborator Author

Meeting 2020-10-15: This issue was discussed together with #933. TL;DR - Order of operations throughout Temporal will be largest units first, even for subtraction of positive durations or adding negative durations. For more details, see #933.

@justingrant
Copy link
Collaborator Author

justingrant commented Oct 16, 2020

I'm going to close this issue because action items are all in #993.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

4 participants