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

difference() should throw if arguments are in the "wrong" order #663

Closed
justingrant opened this issue Jun 11, 2020 · 2 comments · Fixed by #667
Closed

difference() should throw if arguments are in the "wrong" order #663

justingrant opened this issue Jun 11, 2020 · 2 comments · Fixed by #667

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jun 11, 2020

Currently, difference() returns the absolute value of the difference between two Temporal instances. I'm concerned that this behavior will lead to bugs in cases where a developer believes that two values are always going to be in a particular order, while in reality they can sometimes unexpectedly be in the opposite order.

Here's a few examples where this can happen:

  • During the hour after DST ends, a later Absolute can yield an earlier DateTime, while during all other hours of the year this is impossible.
  • A real-world event can sometimes unexpectedly be reversed. For example, in rare cases a post-mortem meeting could happen before the end of a sprint, e.g. if a team lead is about to go on a long vacation.
  • Different developers on the same team may have different assumptions about ordering. For example, one module of a financial application package could assume that payments are always dated after invoices, but later another developer working on a different part of the app (who's unaware of that ordering assumption) could add a "revise invoice" feature that could push the invoice date to be after the payment date.
  • Time-series data from an external source that normally arrives in real time could be delayed, breaking assumptions about ordering. I've seen this in the past in cases where failed systems are restored from backups and their data replayed out-of-order. I've also seen it where external systems don't handle DST correctly, where network hiccups caused delayed delivery, etc.

FWIW, I've worked with teams that have run into all of the above issues in the wild.

Temporal can't stop developers from making ordering assumptions that turn out to be wrong, but Temporal can help developers find these bugs easier by making wrong-order difference bugs easier to find.

With the current behavior, difference() will return a normal-looking result and the bug will likely propagate downstream and will be hard to diagnose, especially if the difference is small.

My suggestion is that difference() should enforce the order of its arguments by throwing if passed arguments in the "wrong" order.

Other benefits:

  • Removing the ambiguous order of arguments to Time.prototype.difference() would make it unnecessary to limit results to 12 hours. Max of 12 hours from Time.prototype.difference is confusing #597
  • It aligns to Calendar.prototype.difference that already requires ordered arguments.
  • If we later decided to offer negative durations (No negative durations? #558) in response to feedback, it would be a much less impactful change to existing users because they wouldn't have to change the order in working code-- only the exception path would behave differently. Similarly, if we get more feedback that throwing was a mistake, then it's easy to undo with a relatively non-breaking change.

If we do enforce an order, I'd suggest the other argument must be smaller. This mirrors subtract and compare where if the larger value is first then a positive result is returned. difference would work the same, except that negative values are treated as errors. Given that, without negative durations, compare and difference will often need to be called as a pair, having them be in the same order in would be helpful.

This issue emerged from the 11 June champions meeting discussion.

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 12, 2020
Fixes tc39#663. Fixes tc39#597.  Changes `difference()` to throw if `other`is
larger than `this`. Goal: encourage feedback about long-term solution:
option 1 - previous behavior: `difference()` returns an absolute value
option 2 - behavior in this commit: throw if other is larger
option 3 - negative durations (tc39#558)

Tests are updated to make it easy to choose any of the three options.
Tests for option 1 are commented out in case we want to revert.
The rest of test changes were reversing arguments so that choosing any
of the options will break few tests.
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 12, 2020
Fixes tc39#663. Changes `difference()` to throw if `other`is
larger than `this`. Goal is encourage feedback about long-term solution:
option 1 - previous behavior: `difference()` returns an absolute value
option 2 - behavior in this commit: throw if other is larger
option 3 - negative durations (tc39#558)

Tests are updated to make it easy to choose any of the three options.
Tests for option 1 are commented out in case we want to revert.
The rest of test changes were reversing arguments so that choosing any
of the options will break few tests.

Because `difference` is now unidirectional, it doesn't make sense to
restrict `Time.prototype.difference` to 12 hours max. Fixes tc39#597.
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 12, 2020
Fixes tc39#663. Changes `difference()` to throw if `other`is
larger than `this`. Goal is encourage feedback about long-term solution:
option 1 - previous behavior: `difference()` returns an absolute value
option 2 - behavior in this commit: throw if other is larger
option 3 - negative durations (tc39#558)

Tests are updated to make it easy to choose any of the three options.
Tests for option 1 are commented out in case we want to revert.
The rest of test changes were reversing arguments so that choosing any
of the options will break few tests.

Because `difference` is now unidirectional, it doesn't make sense to
restrict `Time.prototype.difference` to 12 hours max. Fixes tc39#597.
@ptomato
Copy link
Collaborator

ptomato commented Jun 12, 2020

Another option for Temporal.Time.protoype.difference specifically, could be that when the order of the arguments is reversed, the cross-midnight difference is calculated, and when they are not reversed, the same-day difference is calculated. This seems logical to me, and easier than going through withDate/getTime, but a disadvantage is that it seems obscure and undiscoverable.

@justingrant
Copy link
Collaborator Author

justingrant commented Jun 12, 2020

@ptomato a disadvantage is that it seems obscure and undiscoverable.

Yep. Another way to address this could be to offer a new opt-in option, e.g. {disambiguation: 'wrap'} or {overflow: 'wrap'}so users who want Time wrapping could opt into it. Having an option seems closer to the Temporal way of doing things, vs. enforcing one a default one way or the other. The same option could go on Time.prototype.plus and Time.prototype.minus.

But honestly I don't feel strongly about these issues either way. Time is weird, and the withDate workaround seems pretty straightforward and discoverable without the same risks.

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 13, 2020
Fixes tc39#663. Changes `difference()` methods to throw if `other`is
larger than `this`. Goal is encourage feedback about long-term solution:
option 1 - previous behavior: `difference()` returns an absolute value
option 2 - behavior in this commit: throw if other is larger
option 3 - negative durations (tc39#558)

Tests are updated to make it easy to choose any of the three options.
Only a few tests were functionally changed (e.g. to add throws() tests).
The rest of test changes were reversing arguments so that choosing any
of the options later will break relatively few tests.

Because `difference` is now unidirectional, it doesn't make sense to
restrict `Time.prototype.difference` to 12 hours max. Fixes tc39#597.
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 13, 2020
Fixes tc39#663. Changes `difference()` methods to throw if `other`is
larger than `this`. Goal is encourage feedback about long-term solution:
option 1 - previous behavior: `difference()` returns an absolute value
option 2 - behavior in this commit: throw if other is larger
option 3 - negative durations (tc39#558)

Tests are updated to make it easy to choose any of the three options.
Only a few tests were functionally changed (e.g. to add throws() tests).
The rest of test changes were reversing arguments so that choosing any
of the options later will break relatively few tests.

Because `difference` is now unidirectional, it doesn't make sense to
restrict `Time.prototype.difference` to 12 hours max. Fixes tc39#597.
ptomato pushed a commit that referenced this issue Jun 15, 2020
…pe.difference` to 12 hours (#667)

* difference: throw out-of-order; zap Time 12h limit
Fixes #663. Changes `difference()` methods to throw if `other`is
larger than `this`. Goal is encourage feedback about long-term solution:
option 1 - previous behavior: `difference()` returns an absolute value
option 2 - behavior in this commit: throw if other is larger
option 3 - negative durations (#558)

Tests are updated to make it easy to choose any of the three options.
Only a few tests were functionally changed (e.g. to add throws() tests).
The rest of test changes were reversing arguments so that choosing any
of the options later will break relatively few tests.

Because `difference` is now unidirectional, it doesn't make sense to
restrict `Time.prototype.difference` to 12 hours max. Fixes #597.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants