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.

Sign up for GitHub

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

Jump to bottom

Max of 12 hours from Time.prototype.difference is confusing #597

Closed
justingrant opened this issue May 20, 2020 · 2 comments · Fixed by #667
Closed

Max of 12 hours from Time.prototype.difference is confusing #597

justingrant opened this issue May 20, 2020 · 2 comments · Fixed by #667
Labels
feedback
Milestone
Complete design d...

Comments

@justingrant
Copy link
Collaborator

justingrant commented May 20, 2020 •
edited
Loading

#330 proposed and #386 implemented a change to limit Time.prototype.difference to 12 hours.

This makes it hard to correctly answer use-cases like the ones below using the Time type:

  • I have a recurring 14-hour shift every Friday from 6am-8pm. If I make $20 per hour and $30 in overtime after 8 hours, how much will I get paid on those days?
  • How many hours of daylight did we have today?

I see several problems with the current behavior:

  1. Many more real-world activities (and hence software to model those activities) take place during the day than happen overnight. There are certainly use-cases (and hence software) that wrap around midnight, but these are relatively unusual compared with long daytime use-cases.
  2. Developers who need to wrap midnight already know that this is an unusual case, so they'll expect to have to do a little extra work to calculate an across-midnight time difference. Users calculating a 13-hour period during the day won't have this expectation so are unlikely to guard against the 12-hour limit.
  3. Many real-world durations are negative, which requires calling both compare and difference and using this pair in future calculations. Having difference observe the wraparound behavior but compare not doing so will cause bugs if developers expect both to be consistent.
  4. Wrapping around midnight implies knowledge of out-of-bounds data. If a developer chooses a Time type, they're explicitly deciding to ignore times that are outside of a single solar day, so it seems like we should respect that choice.
  5. Time.prototype.difference will return different results than Date.prototype.difference for the same times on the same date. This will not be expected in most use cases, especially given that Temporal makes it easy to split and join dates and times.

I'd suggest reverting this change.
@pipobscure
Copy link
Collaborator

I agree. This seems strange as

0:00 is smaller than 23:59 and therefore the difference is larger.

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2020

If we revert that change then you can still opt-in to the old behaviour using withDate() to specify whether the times are on the same day or not.

@justingrant justingrant mentioned this issue May 29, 2020
Closed
@ptomato ptomato added this to the Stable API milestone Jun 4, 2020
@justingrant justingrant mentioned this issue Jun 11, 2020
Closed
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 12, 2020
@justingrant
Throw if difference() is out-of-order
6a87a18 
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
@justingrant
difference: throw out-of-order; zap Time 12h limit
2c9b046 
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 justingrant mentioned this issue Jun 12, 2020
Merged
justingrant added a commit to justingrant/proposal-temporal that referenced this issue Jun 12, 2020
@justingrant
difference: throw out-of-order; zap Time 12h limit
6ff4c9a 
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 13, 2020
@justingrant
difference: throw out-of-order; zap Time 12h limit
8984f3a 
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
@justingrant
difference: throw out-of-order; zap Time 12h limit
9bec1d4 
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
@justingrant
Throw if difference() is out-of-order & stop limiting `Time.prototy…
d84653e 
…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
Assignees
No one assigned
Labels
feedback
Projects
None yet
Milestone
Complete design decisions
4 participants
@justingrant @pipobscure @ptomato @ryzokuken