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.

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

Round a Temporal.Duration to the nearest small unit #337

Closed
sffc opened this issue Jan 31, 2020 · 13 comments
Closed

Round a Temporal.Duration to the nearest small unit #337

sffc opened this issue Jan 31, 2020 · 13 comments
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation enhancement meeting-agenda non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@sffc
Copy link
Collaborator

sffc commented Jan 31, 2020

There is a need identified in https://github.com/younies/proposal-intl-duration-format/issues/2 to perform rounding of Temporal.Duration fields. For example, if you have a duration { seconds: 59, milliseconds: 999 }, and you want it rounded to seconds, then you should get { minutes: 1 }.

Could we add a .roundTo(field) method that takes a string field like "seconds" (returning a new Temporal.Duration)? That method would probably need a disambiguation parameter.

CC @younies

Related: #324

@ptomato
Copy link
Collaborator

ptomato commented Apr 3, 2020

Should this be part of this proposal or part of DurationFormat?

I wouldn't be opposed to adding this API but we could also consider adding smallestUnit (for rounding) and largestUnit (for balancing) to the options bag in Duration.from(), e.g.

const duration = Temporal.Duration.from({ seconds: 59, milliseconds: 999 })
Temporal.Duration.from(duration, { smallestUnit: 'seconds' }).toString() // => 'PT60S'
Temporal.Duration.from(duration, { smallestUnit: 'seconds', disambiguation: 'balance' }).toString() // => 'PT1M'

@sffc
Copy link
Collaborator Author

sffc commented Apr 3, 2020

It's certainly up for debate about whether this functionality belongs in Intl.DurationFormat or Temporal.Duration.

I've made the case previously that it belongs in Temporal.Duration. Intl.DurationFormat should be a "pass-through" formatter; it should take what it's given and format it. Maybe it can mask fields, but it feels beyond the scope of Intl.DurationFormat to start balancing things between fields. Temporal.Duration puts the math all in one place.

That being said, I can also see arguments that this is Intl.DurationFormat's responsibility. It would be good to have more of those arguments written out.

@sffc
Copy link
Collaborator Author

sffc commented Apr 3, 2020

Another use case: you want to render only the 2 most significant fields. For example, first you tick down hours and minutes, then minutes and seconds (when hours becomes 0), then seconds and milliseconds (when minutes becomes 0).

@younies
Copy link
Member

younies commented Apr 15, 2020

Another use case: you want to render only the 2 most significant fields. For example, first you tick down hours and minutes, then minutes and seconds (when hours becomes 0), then seconds and milliseconds (when minutes becomes 0).

I think this good for counters as @sffc said. But we need to hit a limit. For example, count down for a world cup, but once you hit second. only round for seconds.

@sffc
Copy link
Collaborator Author

sffc commented Apr 15, 2020

A more flexible option might be something more along the lines of:

  • Temporal.Duration.prototype.getSmallestUnit()
  • Temporal.Duration.prototype.getLargestUnit()

Then the decision on exactly which fields to display can be done in user land.

@sffc
Copy link
Collaborator Author

sffc commented May 5, 2020

How about,

const d = Temporal.Duration.from({ days: 10, hours: 23 });
const d1 = d.balance({ largestUnit: "weeks", calendar: "iso" });  // 1 week 3 days 23 hours
const d1 = d.balance({ smallestUnit: "days" });  // 11 days (or 10 days?)

If doing math with weeks or higher, you just have to pass in which calendar you want.

@ryzokuken ryzokuken added behavior Relating to behavior defined in the proposal enhancement non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved and removed meeting-agenda labels May 20, 2020
@ryzokuken ryzokuken added this to the Stable API milestone May 20, 2020
@justingrant
Copy link
Collaborator

justingrant commented May 28, 2020

I've made the case previously that it belongs in Temporal.Duration. Intl.DurationFormat should be a "pass-through" formatter; it should take what it's given and format it. Maybe it can mask fields, but it feels beyond the scope of Intl.DurationFormat to start balancing things between fields. Temporal.Duration puts the math all in one place.

Agreed. Rounding is a business-logic issue. It doesn't only matter when formatting, but also when storing, calculating, etc. For example, some databases allow you to control the precision of date/time data... so the app needs to round before storing. Or sometimes it's important to know if two durations are "equal" but you'll only know that after rounding.

@gibson042
Copy link
Collaborator

I don't think Temporal should conflate balancing (which carries excess quantities into a largest unit, preserving total magnitude) with rounding (which replaces total magnitude with a nearby value that is an exact multiple of some increment). Even the inputs are different—balancing is dependent upon the choice of largest unit, while rounding is dependent upon the choice of increment¹ and mode².

That said, rounding itself is clearly valuable, to both Intl.DurationFormat and to authors. It could certainly be built on top of a minimal Temporal.Duration, but user-space implementations are likely to miss the nuance. I support including it.

¹ which is not necessarily a unit; it could be e.g. "one hundredth of a second" or "6 minutes"
² of which there are many, e.g. IEEE-754-defined round towards 0/−∞/+∞ and round to nearest with ties to even/away from 0, and obvious extensions round away from 0 and round to nearest with ties to odd/0/−∞/+∞—and the list is even longer if non-pure options are allowed.

@ptomato ptomato modified the milestones: Stable API, Stage 3 May 28, 2020
@ptomato
Copy link
Collaborator

ptomato commented May 28, 2020

Meeting, May 28: We decided not to add this API to the initially shipping polyfill at the last minute, because it needs more design, but there is consensus that we do need something like it.

@justingrant
Copy link
Collaborator

I agree with @gibson042 that balancing and rounding are fundamentally different operations. But I can also see value in combining both operations into a single method because the problem domains overlap. For example, dateTime.plus(other, {smallestUnit: 'months'} and dateTime.plus(other, {largestUnit: 'days'}) both have to deal with the similar complexities around calendars. Ditto for hours vs. days and DST.

Another advantage of co-location: it's a common use case to only want a single unit, e.g. "total days" or "total minutes". In cases where largestUnit===smallestUnit, a more ergonomic API may be possible, e.g. units: 'hours' vs. units: {smallest: 'seconds', largest: 'hours') or even crazy use cases like skipping intermediate units e.g. units: ['years', 'days']. I'm not saying all these use cases are needed or even desirable in the API, only that combining the operations opens up flexibility to tackle different use cases ergonomically.

Invalid combinations (e.g. {largestUnit: 'hours', smallestUnit: 'days') would also be easier to prevent if the operations were co-located in the same method.

If we did decide to co-locate balancing and rounding, we'd want a single term that encompasses both, e.g. "normalize", "coerce", "adjust", "limit", "clamp", "trim", "condense", etc.

Ironically, in #609 I made the opposite argument about assignment vs. addition/subtraction on non-Duration types. Currently we allow both operations in a single with/from method via disambiguation: 'balance'. But unlike rounding and balancing, I was unable to think of any non-Duration use cases where combining the two operations in one method call results in clearer code.

@justingrant
Copy link
Collaborator

justingrant commented Jul 30, 2020

FYI - this issue is addressed by #789 which is a concrete proposal to address duration rounding and other related topics.

Please feel free to review #789 and provide feedback.

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

This is now being tracked in #856, which was split off from #789.

@ptomato ptomato added the documentation Additions to documentation label Sep 18, 2020
@ptomato
Copy link
Collaborator

ptomato commented Sep 18, 2020

Actually, shall we just close this in favour of #856 instead.

@ptomato ptomato closed this as completed Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation enhancement meeting-agenda non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

6 participants