tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.34k stars 152 forks source link

Proposal: rounding method & total method for Duration type #789

Closed justingrant closed 4 years ago

justingrant commented 4 years ago

This proposal:

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)

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 it round as a placeholder, but we'll probably want to bikeshed on the name.

2. This method will have only one optional parameter: an options bag. Options should do the following things:

3. smallestUnit / largestUnit options

4. relativeTo option

5. Rounding option(s?)

6. Weeks vs. Days/Months

7. Remainder-only option?

8. Usage in Duration.prototype.plus and Duration.prototype.minus

9. total method for Duration type

10. Should we offer timePortion, 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 and Duration.prototype.timePortion.

11. Should balancing be removed from Duration's from and with?

12. Rounding for non-Duration Temporal types

13. Balancing in non-Duration difference methods

sffc commented 4 years ago

2.2 Control rounding at the low end. Rounding types include: nearest; floor (round towards zero); ceiling (round up; away from zero).

We should probably use rounding modes: https://github.com/tc39/proposal-intl-numberformat-v3/issues/7

4.5 @sffc noted a case where users may want to assume 30-day months to avoid specifying a start date. Should we support this case via a custom calendar? IMHO this would be confusing because how would months roll up into years? Instead, I think a reasonable solution for this case is having users set largestUnit: 'days' and then manually convert to months as needed.

SGTM

4.7 If largestUnit is 'days' or larger, then relativeTo is required. Otherwise it's ignored.

But you also said that an undefined relativeTo implies largestUnit="days"?

5.6 Do we want to offer a "multi-unit" rounding, e.g. "nearest 15 minutes", "number of quarters (3-month periods)"? If yes, two possible syntaxes are below. I prefer the former for clarity and for better IDE discoverability.

Rounding increments are tricky. I have some related ideas sketched out in https://github.com/tc39/proposal-intl-numberformat-v3/issues/8#issuecomment-619304130.

For example, what should be the result of balancing PT400M with an interval of 25 minutes?

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.

6.2 We'll use the largestUnit option for this purpose. If largestUnit is 'months' or 'years', then weeks will be zero in the result and the remainder days will be put into the days property.

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!

justingrant commented 4 years ago

Thanks @sffc for this thorough review. I agree with all your feedback and I updated the proposal accordingly.

But you also said that an undefined relativeTo implies largestUnit="days"?

Shoot, you're right. I reworked section 4.7 to change the default largestUnit to 'hours', which makes the default balancing DST-safe and fixes the contradiction you found.

Right; what happens in the edge case where smallestUnit='weeks' and largestUnit='months'?

Good point. I think "weeks" should be included in that case. I reworked section 6.2 accordingly. Please review.

I'm convinced that the only sane way to do rounding increments is for the increments to be even divisions of the greater unit.

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.

We should probably use rounding modes: tc39/proposal-intl-numberformat-v3#7

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 difference options). LMK what you think.

sffc commented 4 years ago

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?

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.

  1. Rounding for non-Duration Temporal types

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.

12.5 We should probably accept non-plural naming of units because non-Duration types' fields aren't pluralized.

325

sffc commented 4 years ago

Can you double-check the new section 6.2? It doesn't seem to address { smallestUnit: "weeks", largestUnit: "months" }.

justingrant commented 4 years ago

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.

OK sounds good. I left feedback about naming in https://github.com/tc39/proposal-intl-numberformat-v3/issues/7#issuecomment-664717034

Can you double-check the new section 6.2? It doesn't seem to address { smallestUnit: "weeks", largestUnit: "months" }.

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 days and the Duration has both a date and a time portion. So I added a new section:

  • 4.2.4 TODO: Balancing date/time durations with largestUnit: 'days' or largestUnit: '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 for relativeTo that would enable that case, e.g. null or a literal string like 'utc'?
justingrant commented 4 years ago

Decisions 2020-07-31: 1) Add a round method to non-Duration types 2) Add rounding capabilities to non-Duration difference methods 3) Match Intl.NumberFormat V3 for rounding modes (including naming) 4) No consensus on rounding and balancing method on the Duration method. Need more discussion.

We'll move forward on 1-3 and do more discussion on 4.

justingrant commented 4 years ago

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 round and difference of non-Duration types (which has consensus support), and a second part to deal with changes on the Duration type itself.

justingrant commented 4 years ago

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.

justingrant commented 4 years ago

This proposal is now split in two:

I'm going to close this issue. Please add your feedback to those other two issues.