-
Notifications
You must be signed in to change notification settings - Fork 152
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
Proposal: rounding and balancing for Duration type (replaces #789) #856
Comments
Decisions in meeting 2020-09-11:
|
It occurred to me that we don't have to explicitly say that I suggest changing it to:
|
Agreed. Updated. |
I've been trying to figure out what the correct balancing behaviour is for the years-months-weeks-days combination, for each value of It's best expressed in this table:
The strange case is something like: d = Temporal.Duration.from({ years: 1, months; 1, weeks: 1, days: 1 });
d = d.round({ largestUnit: 'weeks', overflow: 'constrain', relativeTo: '2020-01-01' }); You get "P1W398D", i.e. one week and 398 days (366 days in 2020 + 31 days in January 2021 + 1 day in the original duration). What's surprising is that you asked for weeks to be the largest unit, i.e. higher units are balanced down into weeks, but the weeks are left untouched and you get a bunch of days. The other option is "P57W6D", which is maybe less surprising, but then on the other hand violates the principle of I think the former is correct, despite being surprising. |
I admit I haven't fully understood the problem above (will take a closer reading than I have time for tonight) but does this section help?
The referenced info in #827 section (7.7) is:
Does this help resolve the problem you found? |
It doesn't, unfortunately - the difference is that in #827 we are using the options to decide how to express a duration that wasn't previously expressed by the user, so we can simply set weeks to 0 and move on. In Duration.round(), with |
I think we should reconsider this. If we implement this as described, then the method will attempt to balance any existing days, weeks, months, and years, down to hours, and fail because there is no relativeTo. I'd propose making the default largestUnit always years, but just skip the balancing between hours, days, weeks, months, and years if relativeTo is not given. That would make the option simpler to explain to users, as well. |
Sorry I missed this one. I'm off for the rest of the day (wedding anniversary!) but I'll take a look probably tomorrow.
Sent from my mobile device.
…________________________________
From: Philip Chimento <[email protected]>
Sent: Thursday, September 24, 2020 12:51:54 PM
To: tc39/proposal-temporal <[email protected]>
Cc: Justin Grant <[email protected]>; Author <[email protected]>
Subject: Re: [tc39/proposal-temporal] Proposal: rounding and balancing for Duration type (replaces #789) (#856)
3.5 If relativeTo is NOT undefined, then the defaults for for largestUnit and smallestUnit will be the full range of units: {largestUnit: 'years', smallestUnit: 'nanoseconds'}.
* 3.5.1 However, if relativeTo is undefinedthen defaults will be{largestUnit: 'hours', smallestUnit: 'nanoseconds'}`. These defaults avoid variable-length units like months or days in a DST time zone.
I think we should reconsider this. If we implement this as described, then the method will attempt to balance and existing days, weeks, months, and years, down to hours, and fail because there is no relativeTo.
I'd propose making the default largestUnit always years, but just skip the balancing between hours, days, weeks, months, and years if relativeTo is not given. That would make the option simpler to explain to users, as well.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#856 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AACDVXUUG74B47BH5P4R7SLSHOPNVANCNFSM4QNZ2ITQ>.
|
Happy anniversary! No rush, I am partially off today as well so I definitely won't finish this. |
What I suggested above isn't a good solution either, because it brings back the problem where you sometimes get an exception and sometimes don't. The problem is this: d = Temporal.Duration.from({ years: 5, minutes: 5 })
d.round({ smallestUnit: 'hours' })
d.round({ largestUnit: 'hours', smallestUnit: 'hours' }) As a user, what I would expect to get out of line 2 is "5 years". What I would expect to get out of line 3 is an exception, because I asked for the largest unit in the result to be hours and that's not possible without a reference point. But according to what we have currently decided on, the result of both lines will be the same (and it's not clear to me which one it will be.) |
Unless anyone objects soon, here's what I'll proceed with:
This is what will be the result of the examples that I gave in previous comments: d = Temporal.Duration.from({ years: 1, months; 1, weeks: 1, days: 1 });
d.round({ largestUnit: 'weeks', relativeTo: '2020-01-01' }); // => P1W398D
d = Temporal.Duration.from({ years: 5, minutes: 5 })
d.round({ smallestUnit: 'hours' }) // => P5Y
d.round({ largestUnit: 'hours', smallestUnit: 'hours' }) // => exception, relativeTo missing |
Quick poll, what do you expect the answer of this to be? d = Temporal.Duration.from({ months: 1, weeks: 2 })
d.round({ largestUnit: 'days', smallestUnit: 'days', relativeTo: '2020-01-01' }) // => 45 days
d.round({ largestUnit: 'weeks', smallestUnit: 'days', relativeTo: '2020-01-01' }) // => ??? (And do you expect the answer to be different if overflow is balance or constrain?) |
Sorry I forgot to follow up on your questions! For the case above, my expectation was that you'd get But I admit that I haven't thought through the cases for |
Yes, let's do that! I'm about to close up shop for the day, but I'll email you about tomorrow. |
@ptomato and I chatted about the problems he found above. The core issue is how to deal with the case where there needs to be some balancing due to rounding but where the user has the default In that case, there are three possible bad outcomes we could choose from:
Here's some specific proposed changes:
|
I updated the OP proposal with the changes noted above. Search for "2020-10-06" to find the specific sections that were changed/added. |
@ptomato - how is this different from (7) ?
|
Somehow I had missed 7.3. Never mind. |
Removes the 'overflow' option from Duration.add() and Duration.subtract(). This also removes the concept of "balance-constrain." Durations are balanced up to the largest nonzero unit in the input duration, and a different largest unit can be selected with the largestUnit option. The options object in Duration.add() and Duration.subtract() remains for the time being, even though it accepts no valid options, because rounding options will be added in #856. See: #856 Closes: #857
Removes the 'overflow' option from Duration.add() and Duration.subtract(). This also removes the concept of "balance-constrain." Durations are balanced up to the largest nonzero unit in the input duration, and a different largest unit can be selected with the largestUnit option. The options object in Duration.add() and Duration.subtract() remains for the time being, even though it accepts no valid options, because rounding options will be added in #856. See: #856 Closes: #857
Removes the 'overflow' option from Duration.add() and Duration.subtract(). This also removes the concept of "balance-constrain." Durations are balanced up to the largest nonzero unit in the input duration, and a different largest unit can be selected with the largestUnit option. The options object in Duration.add() and Duration.subtract() remains for the time being, even though it accepts no valid options, because rounding options will be added in #856. See: #856 Closes: #857
Here's a checklist of what's left to do on this issue:
|
Implements a Duration.compare() static method with a relativeTo option for comparing durations with date units. Allowing ZonedDateTime as relativeTo does not work yet. See: #856
Removes old behaviour of never converting between years, months, weeks, and days, and replaces it with using relativeTo to do the conversion. If either operand has nonzero years, months, or weeks, then relativeTo is required. See: #856
Implements a Duration.compare() static method with a relativeTo option for comparing durations with date units. Allowing ZonedDateTime as relativeTo does not work yet. See: #856
Implements a Duration.compare() static method with a relativeTo option for comparing durations with date units. Allowing ZonedDateTime as relativeTo does not work yet. See: #856
Removes old behaviour of never converting between years, months, weeks, and days, and replaces it with using relativeTo to do the conversion. If either operand has nonzero years, months, or weeks, then relativeTo is required. See: #856
This operation gives the total number of nanoseconds in an exact duration as a bigint, not losing precision. This will be reused in Duration.compare(). This operation takes an "offset shift" parameter, used if there are days that need to be balanced down to hours. If it's 0 (as is the case everywhere in this commit) then days are always 24 hours. We will start using nonzero values for this in Duration.compare(). See: #856
Implements a Duration.compare() static method with a relativeTo option for comparing durations with date units. See: #856
This operation gives the total number of nanoseconds in an exact duration as a bigint, not losing precision. This will be reused in Duration.compare(). This operation takes an "offset shift" parameter, used if there are days that need to be balanced down to hours. If it's 0 (as is the case everywhere in this commit) then days are always 24 hours. We will start using nonzero values for this in Duration.compare(). See: #856
Implements a Duration.compare() static method with a relativeTo option for comparing durations with date units. See: #856
This operation gives the total number of nanoseconds in an exact duration as a bigint, not losing precision. This will be reused in Duration.compare(). This operation takes an "offset shift" parameter, used if there are days that need to be balanced down to hours. If it's 0 (as is the case everywhere in this commit) then days are always 24 hours. We will start using nonzero values for this in Duration.compare(). See: #856
Implements a Duration.compare() static method with a relativeTo option for comparing durations with date units. See: #856
This issue is based on what remained in #789 after extracting rounding of non-Duration types into #827. The proposal below focuses only on changes to the Duration type itself. Please review #827 before this one.
Proposal Summary
Temporal.Duration
type with around()
method to perform balancing and/or rounding. Most rounding behavior is already defined in Proposal: Rounding method (and rounding fordifference
andtoString
) for non-Duration types #827, so this proposal focuses on what's different when applied to the Duration type.plus
andminus
to ensure that duration arithmetic can be done safely even with units that have variable lengths like months, days (if there's DST), and years (for some non-ISO calendars).compare
method which takes a subset of the options above.Use Cases
Common Use Cases (should have ergonomic solutions for these)
Less Common Use Cases (mostly not supported)
{largestUnit: 'days'}
. The latter is really just a special case of the "Get Remainder" case above.Open Issues
PT2H59M55S
rounded to the nearest minute without balancing would bePT2H60M
. This seems unexpected. Should the default be to balance, and users can opt out of balancing instead of opting in? Or should it always keep balanced durations balanced? EDIT 2020-10-08 Answer: Yes. Output durations will always be balanced except for the largest unit and exceptweeks
will usually be zero. See below for more details.plus
andminus
methods accept alargestUnit
of'months'
or'years'
ifrelativeTo
is omitted? If yes, then there won't be any way to know at development time whether it will fail at runtime. If no, then failures will be found at development time, with the downside of disallowing cases which require no balancing likeP1Y2M + P2Y3M
? My inclination is to disallow these units in this case, to avoid unpredictable runtime behavior. See (4.4) below. EDIT 2020-10-08 Answer: we will require a relativeTo for these cases.Proposal
1. The
Duration
type will add around()
method that generally matches the behavior ofround()
on other types.round
method will have only one optional parameter: anoptions
bag.difference
methods of Absolute, DateTime, etc. See Proposal: Rounding method (and rounding fordifference
andtoString
) for non-Duration types #827 for more info on options behavior and naming.largestUnit
andsmallestUnit
will match the behavior defined in Proposal: Rounding method (and rounding fordifference
andtoString
) for non-Duration types #827 for thedifference
method of DateTime, including behavior affectingweeks
as defined in Proposal: Rounding method (and rounding fordifference
andtoString
) for non-Duration types #827 section (7.7) EXCEPT......the default forsmallestUnit
/largestUnit
options depends on therelativeTo
option.largestUnit
isundefined
or'auto'
then the defaultlargestUnit
will be the largest nonzero unit in the duration forround
or the largest unit in either input forplus
andminus
. In other words, the default behavior is to limit durations to the largest unit that they started out with, with balancing happening underneath that unit. If the caller wants the equivalent of the now-removedoverflow: 'balance'
option to expand the duration beyond its current largest unit, then the caller should explicitly setlargestUnit
(andrelativeTo
if needed).weeks
is nonzero in the input ofround
(or either input ofplus
/minus
) then weeks will be filled in the output if needed. Ifweeks
is zero in the input(s) then behavior matches behavior of non-Duration types which is thatweeks
will only be filled in the output if the user opts in via settinglargestUnit: 'weeks'
orsmallestUnit: 'weeks'
.roundingMode
androundingIncrement
will match the behavior defined in Proposal: Rounding method (and rounding fordifference
andtoString
) for non-Duration types #827, EXCEPT...'floor'
and'trunc
rounding modes will behave differently in Duration, but they behave the same in non-Duration types where zero and negative infinity are considered the same.difference
andtoString
) for non-Duration types #827, the behavior of these options will align with Intl.NumberFormatV3 for decisions around option naming, which rounding modes are supported, etc.round
will add EDIT 2020-10-08 a new option:relativeTo
.two additional options:relativeTo
andoverflow
.2. No
overflow
option EDIT 2020-10-06: removed this option after discussion with @ptomato2.1 Theoverflow: 'balance' | 'constrain'
option behaves like the same option onfrom
.If'balance'
then the resulting duration will be balanced.If'constrain'
(the default) will do no balancing.overflow: 'constrain'
option inplus
andminus
is currently used to control whether the output is fully balanced or minimally balanced. This behavior has a few problems, especially after rounding is in the mix:constrain
matches the most common use case for unbalanced durations which is where only the largest unit is unbalanced, e.g. 45 days, 3 hours, and 10 minutes, or @jasonwilliams's case where the BBC measures all program lengths in seconds. It's not clear that "unbalanced in the middle" durations like "45 days and 180 minutes" are a real use case that we need to support in the results ofplus
,minus
, andround
.overflow
options onplus
,minus
, orround
. Instead, we'll optimize for the "unbalanced largest unit" use case by automatically setting thelargestUnit
default to prevent durations from growing into larger units by default. See (3.5.1) below for more details.overflow
option was already inplus
andminus
before this proposal. It will be removed from those methods.3.
relativeTo
optionP40D
balances toP1M9D
ifrelativeTo
is2020-01-01
.P40D
balances toP1M11D
ifrelativeTo
is2020-02-01
this
sorelativeTo
isn't needed. That's why it was omitted from Proposal: Rounding method (and rounding fordifference
andtoString
) for non-Duration types #827.undefined
, which allows rounding durations withdays
or smaller units without worrying about DST or starting points. This is the default behavior if no options bag is provided.undefined
is used, then all days will be assumed to be 24 hours long.undefined
is used, then a RangeError will be thrown if the duration has a non-zeromonths
oryears
. TS typings should enforce this restriction.undefined
is used, then a RangeError will be thrown if largestUnit in the input is'weeks'
or larger.LocalDateTime.from
, then construct a new LocalDateTime instance from this value.DateTime.from
, then construct a new DateTime instance from this value.3.5 IfrelativeTo
IS NOTundefined
, then the defaults for forlargestUnit
andsmallestUnit
will be the full range of units:{largestUnit: 'years', smallestUnit: 'nanoseconds'}
.3.5.1 However, ifEDIT 2020-10-06: defaults are no longer controlled by relativeTo. See (1.2.1.2).relativeTo
ISundefined
then defaults will be{largestUnit: 'hours', smallestUnit: 'nanoseconds'}
. These defaults avoid variable-length units like months or days in a DST time zone.4. Usage in
plus
andminus
dur1.minus(dur2)
could be replaced byldt.plus(dur1).minus(dur2).difference(ldt)
.round
(includingrelativeTo
) toplus
andminus
too. This would solve the DST issue and would also allow duration arithmetic to include all units including months and years.relativeTo
option is applied tothis
, not toother
. This allows chaining multiple operations with the samerelativeTo
, e.g.dur1.plus(dur2, {relativeTo: ldt}).plus(dur3, {relativeTo: ldt})
.relativeTo
is omitted, thenlargestUnit
must be'weeks'
or smaller, or a RangeError will be thrownrelativeTo
isundefined
, then:largestUnit
will be the largest non-zero unit in either input. (see 3.5.1 for more details)'weeks'
or larger units OR iflargestUnit
isweeks
or larger. This matches behavior ofround
(see 3.3.3.2 and 3.3.3.3) andcompare
(see 5.2).5. Usage with
compare
compare
algorithm is simple: round down nanoseconds and compare the results. In order to compare across unbalanced durations and/or to adjust for DST,compare
will accept arelativeTo
option which will behave identically torelativeTo
in other methods above.relativeTo
is undefined, thencompare
will throw if either duration has a non-zero unit inmonths
oryears
6. No
equals
equals
because equality is ambiguous for unbalanced durations (e.g.PT3600S
vsPT60M
) so we opted to simply supportcompare
which will force users to think a little bit about what the comparison will mean.7. We'll add an
'auto'
value forlargestUnit
in Duration and non-Durationdifference
methods where it's used.'auto'
, likeundefined
, will select the default unit. EDIT 2020-10-06: added this section'auto'
means "the largest nonzero unit in the input(s)"difference
of non-Duration types'auto'
means to pick the default unit of the type:'seconds'
'days'
'hours'
'hours'
'years'
smallestUnit
then setlargestUnit = smallestUnit
.8. EDIT 2020-10-06: added this section
round
will require eithersmallestUnit
orlargestUnit
to be explicitly provided and notundefined
. This is because without one of these two options, the rounding operation would be a no-op. This is analogous to how we requiresmallestUnit
on theround
methods of non-duration types. A RangeError should be thrown if neither is provided.The text was updated successfully, but these errors were encountered: