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

Total methods for Duration? #584

Closed
justingrant opened this issue May 18, 2020 · 12 comments · Fixed by #1064
Closed

Total methods for Duration? #584

justingrant opened this issue May 18, 2020 · 12 comments · Fixed by #1064
Assignees
Labels
behavior Relating to behavior defined in the proposal documentation Additions to documentation enhancement non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

While researching #558, I found that .NET's Temporal.Duration (called TimeSpan) has nice "total" methods, e.g. TotalMilliseconds, TotalHours.

The value returned is a floating point number to capture fractional smaller units.

Should Temporal offer something similar? Per #559 it probably would be best to limit these to hours/minutes/seconds/etc. to avoid DST issues with days.

I know that largestUnit serves this purpose in Temporal today and is helpful for cases where you want multiple units (e.g. 2 days and 10 hours). But IMHO those TotalXXX methods seem like a nice way to handle the single-unit case with better ergonomics and better discoverability via IDE autocomplete, compared to largestUnit which is buried in an optional parameter.

@justingrant
Copy link
Collaborator Author

From #730:

@ptomato: can you say anything more about the use case where you missed this method?

@thojanssens: For example, get the difference between two times in total of seconds. AFAIK Duration doesn't give a value like "total of X".

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

This is going to land with #856, not a dedicated "total" method, but:

totalSeconds = duration.round({ largestUnit: 'seconds', smallestUnit: 'seconds' }).seconds;

Can we close this?

@ptomato
Copy link
Collaborator

ptomato commented Sep 18, 2020

Meeting, Sept. 18: We will close this.

@ptomato ptomato closed this as completed Sep 18, 2020
@justingrant
Copy link
Collaborator Author

justingrant commented Oct 9, 2020

Meeting 2020-10-09: We'll add a total method to support the same use cases as #938: HR time-keeping (measured in fractional hours) and stopwatch timing (measured in fractional seconds).

Caveat: our assumption is that implementing this will be straightforward on top of the work we've already done for round(). If this assumption is wrong and the implementation of total turns out to be much more complicated than expected, then let's revisiit and maybe postpone this to a later release.

Proposed behavior:

  1. Duration will add a new total method which behaves very similarly to round if the same unit is used for largestUnit and smallestUnit, with only the following exceptions:
  • 1.1 It returns a number, not a Duration. This is true even for nanoseconds/microseconds units which can lose precision for larger results.
  • 1.2 The result may include a fractional remainder.
  • 1.3 roundingMode and roundingIncrement are not supported because for those cases, the user should be using the round method. The total method is explicitly about what to use when you're not rounding.
  1. The method will accept a single parameter: an options bag
  2. The unit option will be required and can be the name of any unit of Duration. If omitted, throw a RangeError.
  3. The relativeTo option will have the same behavior as round(), and will be required if the Duration contains a non-zero value for weeks or larger units OR if the unit option is 'weeks' or larger. (AFAIK this matches the expectations of round too)

@justingrant justingrant reopened this Oct 9, 2020
@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved and removed feedback labels Oct 9, 2020
@justingrant
Copy link
Collaborator Author

Here's a prototype userland implementation of total(). I'm planning to PR a real implementation, but @ptomato is making some changes to the rounding algorithm so I was going to wait until his changes land to make sure my proposed approach won't break.

The algorithm:

  1. Calculate a "base duration" by truncating the duration with largestUnit===smallestUnit===options.unit
  2. If unit is nanoseconds then there's no remainder possible so just return the result
  3. Otherwise, calculate an "away duration" using away-from-zero rounding instead of truncation
  4. If base is the same as away, return the unit value because there's no remainder
  5. Otherwise, if relativeTo is undefined, then set it to any new DateTime because only fixed-length units are involved. (If non-fixed-length units were involved, the calls to round above would have failed)
  6. Calculate a "base DateTime" and an "away DateTime" by adding the base and away durations to relativeTo.
  7. Calculate a "duration DateTime" by adding this to relativeTo.
  8. The remainder denominator is the difference in nanoseconds between "base DateTime" and "away DateTime"
  9. The remainder numerator is the difference in nanoseconds between "duration DateTime" and "base DateTime".
  10. Calculate the remainder ratio by dividing the numerator by the denominator and multiplying by the sign of the original duration.
  11. Add the remainder ratio to the base duration's unit value, and return that sum

Steps 6-9 should be reworked after duration.plus gets support for relativeTo, because the only reason to create a DateTime is to work around lack of relativeTo support in duration.plus. A significant downside of creating a DateTime is that it won't work if relativeTo is a ZonedDateTime, so if we switch to using duration.plus then it should "just work" with a ZDT relativeTo.

function total(duration, options) {
  let { unit, relativeTo } = options;
  const base = duration.round({ smallestUnit: unit, largestUnit: unit, roundingMode: 'trunc', relativeTo });
  if (unit === 'nanoseconds') return base.nanoseconds;
  const roundingMode = duration.sign < 0 ? 'floor' : 'ceil'; // TODO: replace with away-from-zero rounding mode
  const away = duration.round({ smallestUnit: unit, largestUnit: unit, roundingMode, relativeTo });
  if (base[unit] === away[unit]) return base[unit]; // No rounding means no remainder. Return the integer.

  // If only time units, any DateTime is OK. Invalid options are caught by .round() above.
  if (!relativeTo) relativeTo = new Temporal.DateTime(1970, 1, 1);
  relativeTo = Temporal.DateTime.from(relativeTo);

  // Calculate remainder, using diff of round-towards-zero and round-away-from-zero as denominator.
  const plusBase = relativeTo.add(base);
  const plusAway = relativeTo.add(away);
  const denominator = plusAway.since(plusBase, { largestUnit: 'nanoseconds' });
  const remainderDuration = relativeTo.add(duration).since(plusBase, { largestUnit: 'nanoseconds' });
  const remainder = duration.sign * (remainderDuration.nanoseconds / denominator.nanoseconds);
  return base[unit] + remainder;
}

@ptomato
Copy link
Collaborator

ptomato commented Oct 26, 2020

I commented on #998, but no need to wait for the changes to the rounding algorithm, as it should only affect this in edge cases.

At first glance this looks correct for a userland implementation, though in the polyfill we should not use round() and instead split out the relevant parts of the rounding algorithm. This is because in the rounding algorithm we already calculate the fractional remainder, in order to round it, so in total() we should be able to do the same thing only without rounding it.

@justingrant
Copy link
Collaborator Author

This is because in the rounding algorithm we already calculate the fractional remainder, in order to round it, so in total() we should be able to do the same thing only without rounding it.

I thought the same thing. But the current round() method does 4 different operations: UnbalanceDurationRelative, RoundDuration, BalanceDurationRelative, and BalanceDuration. With the rounding in the middle I'm not how to decompose the current logic in round to make it work as-is for total(). For rounding the order seems right because if you round up then it will affect balancing so it makes sense to do the rounding step first before balancing. And part of total must involve rounding up and then balancing to calculate the denominator of the remainder. So I'm not sure if any of those steps are optional. If none are optional for total, then is the right solution to convert the core of round (the 4 steps) into an AO that can be used from both round and total?

Or can you see a way to simplify so that fewer operations would be needed for total?

@ptomato
Copy link
Collaborator

ptomato commented Oct 26, 2020

I believe the final two (BalanceDurationRelative and BalanceDuration) are not needed for total(). If you first balance down from the original duration so that all units above largestUnit are 0, then do everything in RoundDuration except actually round the fractional number, then I think you have the correct answer already.

@justingrant
Copy link
Collaborator Author

I believe the final two (BalanceDurationRelative and BalanceDuration) are not needed for total(). If you first balance down from the original duration so that all units above largestUnit are 0, then do everything in RoundDuration except actually round the fractional number, then I think you have the correct answer already.

So RoundDuration will balance all units smaller than smallestUnit? For example, if my duration is an unbalanced one like PT123456789S and I call duration.total({unit: days}) then is RoundDuration be smart enough to balance the seconds into days before truncating the smaller-than-days units?

@ptomato
Copy link
Collaborator

ptomato commented Oct 27, 2020

That's correct.

@justingrant
Copy link
Collaborator Author

no need to wait for the changes to the rounding algorithm, as it should only affect this in edge cases.

Unfortunately it seems to affect more than edge cases. I've implemented total() but it's impossible to get the tests passing because fractional amounts from RoundDuration are mostly wrong because of #1059, specifically because rounding is counting backwards from relativeTo instead of forwards from relativeTo. Do you know when the fix for #1059 will land?

@ptomato
Copy link
Collaborator

ptomato commented Oct 27, 2020

OK, I'll prioritize that one for today.

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 non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants