-
Notifications
You must be signed in to change notification settings - Fork 155
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
No negative durations? #558
Comments
I did find while writing the cookbook that sometimes it was useful to have a signed duration, and it was on my list to open an issue to discuss it when the cookbook was done 😄 Here's the comment in the cookbook thread (#240 (comment)) and down below it is a link to the earlier discussion. |
Yep. Here's a simple use-case I'd recommend for verifying DX of const diff = previousFlight.scheduled.difference(previousFlight.actual);
const estimate = thisFlight.scheduled.add(diff); With only unsigned durations, you need this: const sign = previousFlight.scheduled.compare(previousFlight.actual);
const diff = previousFlight.scheduled.difference(previousFlight.actual);
const estimate = sign < 0 ? thisFlight.scheduled.subtract(diff) : thisFlight.scheduled.add(diff); The latter is:
Another use-case is for a formatting function to display relative time, e.g. "2 days ago" or "in 2 hours". Having this function to accept a single Another case: code often assumes that it "knows" the order of two dates. If that code's assumption is ever wrong, a negative value is more likely to trigger an obvious failure like a crash or unit-test failure because none of the surrounding code or tests expects a negative value. But if a positive value is returned, that's normal behavior so is less likely to be caught before shipping. A special case variation of the "knows the order" case above is handling local times in the hour before and after DST ends. For times in this weird period, it's possible for the Absolute difference to be positive while the DateTime difference can be negative, or vice versa. So even if app code doesn't bother calling |
IIRC @gibson042 has argued for negative durations in the past, though I'm having trouble finding the concrete issue. |
BTW, I found one another place where negative durations came up: @devbww (one of the Google date library maintainers) said:
|
I'd guess this should be achieved with |
@ptomato - Imagine you wanted to write a function that enabled multi-unit display of relative times (e.g. 1 minute and 30 seconds ago). A |
FYI, the latest consensus from the champions group on this subject is what @pdunkel stated here: https://github.com/tc39/proposal-temporal/blob/main/meetings/agenda-minutes-2019-09-10.md
|
It seems very strange to me that what is effectively |
BTW, I did a quick review of how other JS libraries and other platforms handle negative durations. I couldn't find any major library or platform that disallowed negative durations, including libraries like Abseil Time that do duration balancing like Temporal does. Here's the ones that do support negative durations. Did I miss any major platforms or libraries?
This other-library review was an interesting exercise. I found a few good ideas that were unrelated to negative durations, so I'll file them as separate suggestions. |
Here's another case: "I worked from 8:00AM until 10:00PM. How many hours did I work?" `You worked ${new Temporal.Time(22).difference(new Temporal.Time(8)).hours} hours` expected: IMHO, if Temporal doesn't support negative durations, then I'd probably recommend removing arithmetic from the |
This calculation will fail if your workday crosses a DST transition, will it not? I imagine you'd want to use |
Yep, agreed. A variant of that use case where Absolute won't work would be: "I have a recurring 14-hour shift every Friday from 6am-8pm. How much will I get paid on those days?" But my concerns about Time are secondary to the main question of whether Durations should allow negatives. Let's continue Time-related discussion at #597 and focus this issue on whether Duration should be negative. Moment.js, Abseil Time, .NET, Java, JodaTime, iOS/MacOS, Python, C++ STL, Flutter, Rails, etc. all allow negative durations. Are there any other major libraries that prevent negative durations? Note that wanting a positive-only duration is a common use-case, so we could offer an |
I didn't think that was the case... can you provide an example? Regardless, I want to pull in a relevant excerpt from the cookbook comment I just left:
I believe serialization/deserialization is the only substantial objection to signed Temporal.Duration, but it's a pretty big one if it would be the first deviation from ISO 8601 as I suspect. On the other hand, though, forcing authors to construct their own |
There are a number of areas where we are deviating from ISO-8601 string syntax, documented here: https://github.com/tc39/proposal-temporal/blob/main/docs/iso-string-ext.md I think we should not block this feature merely on ISO-8601 string syntax. We can just make another extension to the string syntax and document it. For example, something like |
@gibson042 - are there other libraries that support C? I could only find A and B. Both Java and Luxon seem to support both A and B. My suggestion would be to support only B, for two reasons: 1) it's already used in RFC 5545, while A doesn't seem to be standardized anywhere; 2) I think A introduces a lot of extra complexity for developers without clear real-world use cases, especially for persistence. Details are below, including a specific proposal for how negative durations could work across Temporal. RFC 5545 Durations iCalendar durations are standardized in RFC 5545. AFAIK, Durations in this standard are designed for two use cases: to define the length of meetings in local clock time AND to model the reminder time delta before calendar events. "Snoozed" reminders can show up after an event, so they needed to specify negative durations. This calendar-reminder use case also requires deterministic arithmetic behavior, because it'd be bad for one calendar app showing you a reminder at a different time than another. Therefore the spec defines a specific order of operations (add or subtract larger units before smaller ones) and specific DST behavior. I'll post the DST behavior over in #559 to keep this issue focused on negativity. Unfortunately, the calendar reminder use case (and therefore that standard) also excludes months and years, probably for the same non-determinism reasons that Duration lacks day/month/year balancing and But it seems like a reasonable homestead to follow, especially because that format is also supported by other libraries like Luxon (probably moment too, but not sure) and platforms like Java. It also seems reasonable to follow RFC's 5545 order-of-operations behaviors for date/time math, if only because order-of-operations is somewhat arbitrary and I don't see any reason to not follow the standard. Do you? FWIW, Temporal's current order of operations is inconsistent: // minus current behavior: days first, then months
new Temporal.DateTime(2020, 7, 1).minus({months: 1, days: 1}).toString()
// => "2020-05-30T00:00"
new Temporal.DateTime(2020, 7, 1).minus({months: 1}).minus({days: 1}).toString()
// => "2020-05-31T00:00"
new Temporal.DateTime(2020, 7, 1).minus({days: 1}).minus({months: 1}).toString()
// => "2020-05-30T00:00"
// plus current behavior: months first, then days
new Temporal.DateTime(2020, 6, 30).plus({months: 1, days: 1}).toString()
"2020-07-31T00:00"
new Temporal.DateTime(2020, 6, 30).plus({months: 1}).plus({days: 1}).toString()
// => "2020-07-31T00:00"
new Temporal.DateTime(2020, 6, 30).plus({days: 1}).plus({months: 1}).toString()
// => "2020-08-01T00:00" Adopting RFC 5545's DST behavior is an interesting discussion but IMHO it's out of scope of this issue. Let's discuss over in #559 unless you think there's a reason to combine. Adopting RFC 5545's assumption that all Durations are nominal (aka Why not A? (where each component can have a different sign) Java, Luxon, and (I suspect, but haven't confirmed yet) moment.js support A too. Here's examples from the
I'm skeptical of A, for a few reasons. First, if you don't read the docs you won't know how to resolve this ambiguity: does Second (and probably more important) A in other libraries implies mixed-sign components, so if we went with A then users may expect mixed-sign components in Temporal too. But I don't know any significant, non-contrived use cases for a persisted duration with mixed-sign components. I'd be hesitant to support a non-standardized feature in Temporal without known, important use cases that justify doing so. Some background: duration types typically serve two purposes: to model a real-world time deltas (e.g. reminder time for calendar events, stopwatch time since a particular event, age of a person, etc.) or to provide a property bag format that developers can use to perform math on date/time data. B implies the former. A implies the latter: the duration may not represent a real thing in the real world; it may only represent a serialization of multiple, opposite-sign/different-units math operations that the developer wants to do later. I'm not aware of use cases where that "multiple, opposite-sign/different-units math operations" must be persisted as a single Duration. I can see use-cases for wanting to persist multiple math operations (e.g. an Undo stack in a calendar app), but that seems better modeled as an array of durations, where each duration represents an actual real-world quantity. Also, I'm not even sure that the "multiple, opposite-sign/different-units math operations" case needs any support, even if only in code and not persisted. Duration math is already complicated (balancing, etc.) both conceptually for developers to learn and for implementers of Temporal. Allowing entire durations to be signed is conceptually simple: addition becomes subtraction and vice versa. Implementation is trivial for the same reason. But mixed intra-duration signs are harder to reason about, harder to test, harder to implement, etc. Finally, mixed signs in our API has the same must-read-the-docs ambiguity as the persistence format: does Proposal I thought it'd be helpful to have a specific proposal to discuss, so I wrote one up below:
|
I agree with your reasoning and your suggestions, modulo bikeshedding on names (I have a moderate-to-strong preference for |
Cool! re: |
@justingrant this got lost somewhere, thanks to GitHub's email routing, but for reference: #158 (comment) |
We discussed this issue in the 11 Jun 2020 champions meeting but didn't come to resolution. @pdunkel: here's history about why negative durations were avoided:
@jasonwilliams If negative durations are so widely used, it would be good to understand why. @pdunkel What I heard from other libraries was negatives were put in without a lot of thought, e.g. "if positive, why not negative?". Following other libraries is not a good reason to do it. @sffc (or @gibson042?)
@pdunkel If we wanted to be stricter, then out-of-order arguments to
After thinking about this after the meeting, I've changed my opinion. If there's not consensus that negative durations are the way to go, then it doesn't make sense to make that expensive change now. Instead, I think we get most of the benefit from throwing an exception and punting a final decision until there's more early adopter feedback-- and I think an exception could help us get that feedback. I filed #663 to track this suggestion and I'm happy to PR it. |
@ptomato wasn't at the meeting, but in my mind, the most compelling case in favor of negative durations is #240 (comment): we have concrete data that passing around a We didn't fully have time to discuss the semantics of negative durations in the arithmetic methods, but my suggestion, recorded in the notes, was that the current semantics of |
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.
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.
After today's meeting, I built a PR #667 which will throw if |
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.
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.
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.
…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.
#731 is another bit of feedback in favour of negative durations. |
Today we decided to move forward with negative durations. Please provide feedback on #782 which is the spec for this proposed change. |
In docs for
Duration
, I was surprised that no negative durations were allowed. This limitation seems to make DX much harder for subtractingDuration
s and calculating differences between DateTime/Absolute/Date/Time values, because before doing the subtraction operation you'd need to check which one was greater.This is also different behavior from all date libraries that I'm familiar with, all of which support negative results that can be returned from their equivalent of
difference()
.Would it be possible to relax this rule to support only the largest unit being negative? So you could have "-3 days" but not "3 days and -12 hours".
It's OK if there's no ISO serialization possible for negative durations, especially given that there are already other cases where the value is not ISO serializable.
The text was updated successfully, but these errors were encountered: