-
Notifications
You must be signed in to change notification settings - Fork 154
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 method & total method for Duration type #789
Comments
We should probably use rounding modes: tc39/proposal-intl-numberformat-v3#7
SGTM
But you also said that an undefined relativeTo implies largestUnit="days"?
Rounding increments are tricky. I have some related ideas sketched out in tc39/proposal-intl-numberformat-v3#8 (comment).
I'm convinced that the only sane way to do rounding increments is for the increments to be even divisions of the greater unit. In my NumberFormat V3 proposal above, I restricted the increment to either 5 or 10 in a base-10 system. In the Temporal.Duration case, maybe we can either say that the increments need to be evenly divisible, or explicitly list out all of the allowed increments. For example, if smallestUnit is minutes, then roundingIncrement can be either 1 (default), 2, 3, 4, 5, 6, 10, 12, 15, 20, or 30. Note: I am not expressing an opinion one way or another on whether to include this option.
Right; what happens in the edge case where smallestUnit='weeks' and largestUnit='months'? The rest pretty much looks good to me. Thanks for the detailed write-up! |
Thanks @sffc for this thorough review. I agree with all your feedback and I updated the proposal accordingly.
Shoot, you're right. I reworked section 4.7 to change the default
Good point. I think "weeks" should be included in that case. I reworked section 6.2 accordingly. Please review.
SGTM. There's an added wrinkle in that the # of hours per day is timezone-dependent and the number of months per year is calendar-dependent. I rewrote section 5.6 to address this. Let me know what you think.
Agreed. I updated the spec throughout to say that Temporal should follow the lead of Intl and/or Decimal. When it comes to rounding modes, I assume that Temporal will use the other proposals' naming and modes, except that IMHO the default should be "truncate" for Temporal. See updated section 5.6. A general concern: I strongly agree with you that Temporal should align with rounding options used in other JS proposals, but what are the timelines of Decimal and NumberFormat v3? Which is ahead? Could we be blocked (or could we block those other proposals) while we wait for all three to having options naming & shape finalized? P.S. - I also thought through the impact on non-Duration types and added sections 12 (to add a rounding method to non-Duration types) and 13 (for |
I think whichever proposal progresses first will be the one that decides the terminology. But, I'm hoping to advance Intl.NumberFormat v3 in either September or November. Decimal is still a ways off.
I think this has been brought up before, but we didn't have a concrete proposal for it, so it was never acted upon. Now that we have something to work off of, I agree that this seems like a good thing to do.
|
Can you double-check the new section 6.2? It doesn't seem to address |
OK sounds good. I left feedback about naming in tc39/proposal-intl-numberformat-v3#7 (comment)
Hmm, not sure if the problem is my unclear text but correct solution, or unclear clear text and wrong solution. ;-) Let's see if we can figure out which is the case. Here's how I was thinking it'd work: const dur60 = Temporal.Duration.from('P60D');
const dur61 = Temporal.Duration.from('P61D');
dur60.balanced({
relativeTo: Temporal.Date.from({ year: 2020, month: 1, day: 1 }),
smallestUnit: 'weeks',
largestUnit: 'months',
rounding: 'ciel'
});
// => P2M (no remainder; 60 days = 31 + 29 = 2 months exactly )
dur61.balanced({
relativeTo: Temporal.Date.from({ year: 2020, month: 1, day: 1 }),
smallestUnit: 'weeks',
largestUnit: 'months',
rounding: 'ciel'
});
// => P2M1W (extra remainder day rounded up to 1 week)
dur61.balanced({
relativeTo: Temporal.Date.from({ year: 2020, month: 1, day: 1 }),
smallestUnit: 'weeks',
largestUnit: 'months',
rounding: 'trunc'
});
// => P2M (extra remainder day is truncated) Does this behavior seem correct? If so, how would you recommend changing 6.2 to clarify? If not, what do you think would be correct behavior? BTW, one thing I noticed looking at the sample was lousy ergonomics in the case where largestUnit is
|
Decisions 2020-07-31:
We'll move forward on 1-3 and do more discussion on 4. |
I updated the proposal text to match the decisions above. It may make sense to split this proposal into two pieces: one piece for rounding in |
Following up from our 2020-07-31 meeting, I moved the parts of this issue that had consensus (the parts focused on rounding for non-Duration types) into a new proposal: #827. Next, I'll revise this proposal to shrink its scope to focus only on the Duration type. This will hopefully help us resolve open issues more easily with a smaller scope. |
This proposal is now split in two:
I'm going to close this issue. Please add your feedback to those other two issues. |
This proposal:
Temporal.Duration
type with a new method (calledround()
as a placeholder) to perform balancing and rounding.total(unit, options)
method on Duration to handle common use cases that are difficult to accommodate into abalanced
method.round
) to non-Duration types'difference
methods.round
method to non-Duration types to support rounding of those types, using similar options asround
on Duration.This proposal fleshes out ideas previously discussed in #337 and #584, and discussed at length between @sffc, @jasonwilliams, and @justingrant at the 2020-07-23 champions meeting.
Use Cases
Common Use Cases (we probably want API support for these)
Less Common Use Cases (may choose to not offer API support for these if supporting them makes common use cases more complex)
{largestUnit: 'days'}
. The latter is really just a special case of the "Get Remainder" case above.Challenges / Notes
Below is a summary of problems discussed in the 2020-07-23 champions meeting. This list is likely incomplete, but all of the items below are addressed in this proposal.
Proposal
1. The
Duration
type will add a new prototype method for rounding and balancing. For purposes of this proposal we'll call itround
as a placeholder, but we'll probably want to bikeshed on the name.balanced
,adjusted
,normalized
,transformed
,reshaped
,adapted
,conformed
.balanced
wouldn't be a good name and something more generic, e.g.adjusted
ornormalized
would make more sense. See (12) below for more discussion.2. This method will have only one optional parameter: an
options
bag. Options should do the following things:Duration.prototype.minus
andDuration.prototype.plus
(remember thatplus
with negative durations is the same asminus
). When designing the options for the new balancing method, we should keep plus/minus in mind to ensure consistency.3.
smallestUnit
/largestUnit
optionslargestUnit
property acts like it does in other types'difference
methods.smallestUnit
property defines where rounding happens. All smaller units will be zero in the result.smallestUnit
is larger thanlargestUnit
, then a RangeError should be thrown.total
method, a simple "total of one unit" use case is supported by supplying the same unit name for bothlargestUnit
andsmallestUnit
.smallestUnit
/largestUnit
options depends on therelativeTo
option (see below).4.
relativeTo
optionP40D
would balance toP1M11D
if the starting point is2020-02-01
.Temporal.LocalDateTime
instance (for date/time balancing) or aTemporal.Date
instance (for date-only balancing).undefined
or omitted thensmallestUnit
should default to'nanoseconds'
andlargestUnit
should default to'hours'
. This is like current Duration behavior, exceptdays
andweeks
are not supported.undefined
or omitted andlargestUnit
is'days'
or larger, a RangeError should be thrown.smallestUnit
should default to'nanoseconds'
andlargestUnit
should default toyears
.smallestUnit
should default to'days'
and'largestUnit'
should default to'years'
.smallestUnit
smaller than'days'
.largestUnit: 'days'
orlargestUnit: 'weeks'
where 24-hour-day, ignore-DST behavior is desired will be a common use case. It's a PITA to have to create a dummy LocalDateTime instance in UTC tim zone just to get that behavior. Should there be an ergonomic shortcut value forrelativeTo
that would enable that case, e.g.null
or a literal string like'utc'
?largestUnit: 'days'
and then they can manually convert to months as needed.relativeTo
include:start
,startFrom
,startingPoint
. I preferrelativeTo
because it feels more unidirectional whilestart*
might be ambiguous when working with a negative duration. Feel free to suggest something better.largestUnit
is'days'
or larger, thenrelativeTo
is required. Otherwise it's ignored.relativeTo
is only needed for balancing across the hours/days, days/months, or months/years boundaries, so we could only require it for values that cause an overflow to a larger unit. But IMHO it seems brittle to vary behavior based on the magnitude ofthis
, because failures might only happen in production after some threshold is reached, e.g. 24 hours of gameplay. Instead, I think the better approach is to require a starting point based only on the unit options provided.5. Rounding option(s?)
Math
methods:'ceil'
,'floor'
, and'trunc'
. This will both be familiar to users and will avoid ambiguity from using words like "up" or "largest" when using negative durations. For rounding-to-nearest, see What rounding modes to include? proposal-intl-numberformat-v3#7 (comment) for a discussion of naming challenges. Regardless, we should follow Decimal/Intl's lead if they choose different names.Duration
supported fractional values on its smallest unit (like ISO 8601 does) then we'd also want a "none" (or "float" or "decimal" or "remainder" or "fractional") choice that doesn't do any rounding but instead calculates a fractional remainder value and returns a floating-point number as the lowest unit? But Duration only supports integer fields, so the result would have to be a Duration-like property bag, not a Duration instance, which IMHO would be very confusing. But this is still a valid use case. My suggestion would be to offer a separatetotal
method which could support this. See below.Duration.prototype.toString()
6. Weeks vs. Days/Months
largestUnit
is'months'
or'years'
andsmallestUnit
is'days'
or smaller, then weeks will always be zero and only days and/or months will be included in the result. Edge cases that are NOT ambiguous are noted below.smallestUnit
is'weeks'
, then there is no ambiguity and any remainder will go intoweeks
.largestUnit
is'days'
, then there is no ambiguity because weeks are excluded andweeks
will be zero.{ largestUnit: 'weeks', smallestUnit: 'days' }
then there is also no ambiguity so both fields will be populated.7. Remainder-only option?
largestUnit
, those use cases simply want to truncate some larger units. I've not been able to figure out a good solution to these use cases within theround
API, and given these use cases' lower priority I'm not sure they need API support. Feel free to suggest ideas.8. Usage in
Duration.prototype.plus
andDuration.prototype.minus
balanceConstrain
option because mixed-sign units are not supported, even with negative durations. Therefore,plus
andminus
will be extended to support all options allowed forround
.relativeTo
option, which is the only balancing option that is strictly required for duration arithmetic. But IMHO it'd be clearer and more consistent to allow all balancing options inplus
/minus
, as opposed to just cherry-picking some options but not others.plus
andminus
must support the same options (Suggestion: consistent balancing behavior for Duration'splus
andminus
methods #645). I think the cleanest and most consistent way to support this alignment would be to offerdisambiguation
option values ofconstrain
,balance
, andreject
.constrain
would act likebalanceConstrain
does today. Note that this is a breaking change to current API.relativeTo
option is potentially ambiguous. For example, when addingPT30D
andPT60D
, isrelativeTo
mean the instant before the first Duration, or the instant after the first Duration but before the second duration?relativeTo
to the former meaning: the instant before the first duration is applied. There are two reasons to use this default: 1) it matches howrelativeTo
will be used in theround
method. 2) It enables 3+ durations to be summed while using the samerelativeTo
value for each call.9.
total
method for Duration typelargestUnit
to the same assmallestUnit
, this solution isn't very discoverable nor ergonomic.total
method that takes a requiredunit: string
parameter and an optionaloptions
parameter.options
parameter should only accept therelativeTo
androunding
options.relativeTo
should default toundefined
fortotalHours()
and smaller units' methodsrelativeTo
should be required (no default) fortotalDays()
and larger units' methods.rounding
should default to whatever the default chosen in Intl.NumberFormatV3.10. Should we offertimePortion
,datePortion
properties?* 10.1 RFC 5545 durations always require splitting the date and time portions of a duration. It may be helpful to provide convenience properties for these use cases, e.g.Duration.prototype.datePortion
andDuration.prototype.timePortion
.11. Should balancing be removed from Duration's
from
andwith
?round
method, should we retain balancing infrom
andwith
? Given that we have to have balancing inplus
andminus
, IMHO it seems most consistent to leave balancing infrom
andwith
too.disambiguation: 'balance'
for non-Duration types #609 (fixed by Remove{disambiguation: 'balance'}
inwith()
andfrom()
of non-Duration
types #642), we removed balancing from non-Duration types because it wasn't needed. But with Duration, balancing is inherently required, so there's not the same value in removing balancing.from
.round
method offers.12. Rounding for non-Duration Temporal types
round
method on non-Duration typessmallestUnit
and any other rounding options (e.g. mode, increment, etc.).relativeTo
andlargestUnit
would be ignored.smallestUnit
would be the smallest unit of the underlying type.13. Balancing in non-Duration
difference
methodsdifference
methods throughout Temporal accept alargestUnit
option.smallestUnit
,relativeTo
,rounding
, etc.).The text was updated successfully, but these errors were encountered: