Duration.prototype.round spec text has unused variables, errors

[cjtenny]

and they might be semantically important :/

The specification text defines but doesn't use either of smallestUnitPresent or largestUnitPresent.
In the polyfill and docs, it is specified that at least one of the two must be present or a RangeError will be thrown.

There is currently a discrepancy between the polyfill, docs, and spec text in that the following snippet throws. The polyfill throws, the docs say it shouldn't throw but that one is required, and the spec doesn't say that either is required.

Temporal.Duration.from({days: 1}).round({largestUnit: 'auto'})
// Uncaught RangeError: at least one of smallestUnit or largestUnit is required
//     at Duration.round

Obviously, largestUnit is provided. Also, according to spec text, this shouldn't throw (there is no check that both are present or specification to throw) - the spec would use default values for both smallestUnit and largestUnit if passed an empty options bag, but would throw if not passed any options.

What should we do here?

Were the spec text to be updated to throw if one were present, N.B. that ToLargestTemporalUnit doesn't distinguish between 'auto' being provided and auto' being the default, so the unused computation of largestUnitPresent is still incorrect.

Does it make sense to require non-default for largestUnit but not for smallestUnit? (i.e. the current polyfill state; the default of 'nanoseconds' works: the snippet Temporal.Duration.from({days: 1}).round({smallestUnit: 'nanoseconds'}); does not throw). My belief is that the version espoused in the docs is the best version, which would require a small spec text change.

There are also some errors in the spec text that should be fixed at the same time:

• ToSmallestTemporalDurationUnit is called with the wrong # of arguments; should probably be (options, undefined, « »)
• ToLargestTemporalUnit is called with the wrong # of arguments; should probably be (options, « », undefined)

[cjtenny]

If the version espoused in the docs is what we want to do, then #1380 should implement that and is ready for review.

[ptomato]

I'm looking at #856 but I don't think this was explicitly mentioned, maybe obliquely in §1.2.1.2. However, I think the rationale for requiring smallestUnit in the other types' round() methods was that if you didn't pass any options, it would always be a no-op, so it was likely not what you intended. However, that's not the case for Duration: Temporal.Duration.from({ days: 1, minutes: 90 }).round({ largestUnit: 'auto', smallestUnit: 'nanoseconds' }) should give 1 day, 1 hour, and 30 minutes, so it makes sense that Temporal.Duration.from({ days: 1, minutes: 90 }).round() would as well. So my view is that we should go with the behaviour in the spec text and fix the others accordingly.

@justingrant As the originator of Duration.round what do you think?

[justingrant]

Good catch @cjtenny! The underlying goal of throwing here is to prevent unintended no-ops via round({}) or round(). If the user wants a no-op they can get it, but they'll have to explicitly opt in via largestUnit: 'auto' and/or smallestUnit: 'nanoseconds'. As @ptomato notes above:

if you didn't pass any options, it would always be a no-op, so it was likely not what you intended

So here's what I'd expect:

d = Temporal.Duration.from({ days: 1, minutes: 90 });
d.round({ largestUnit: 'auto', smallestUnit: 'nanoseconds' }); // => OK
d.round({ largestUnit: 'auto' }); // => OK
d.round({ smallestUnit: 'nanoseconds' }); // => OK
d.round({}); // => throws
d.round(); // => throws

I think (not 100% sure though) that the cases above and their expected results match the docs, but not the polyfill nor the spec. @cjtenny and @ptomato - does that match what you see?

The reason why round({ largestUnit: 'auto', smallestUnit: 'nanoseconds' }) shouldn't throw, even though both options are in theory the default values, is because all code that dynamically sets options should be allowed to succeed. For example, imagine a UI where largestUnit and smallestUnit are filled by the user's choices in two dropdown select boxes. If the user chooses the default values from the dropdown, I wouldn't expect throwing because the user is opting into those values. But if the developer just calls round() on its own with no options, that's clearly a bug because it will never do anything. Hence the difference in behavior between omitted options and caller-supplied options that match the defaults.

BTW, this line in the docs is the critical one:

A largestUnit value of 'auto', which is the default if only smallestUnit is given

My intent was that the default of largestUnit is undefined if no smallestUnit is present in the options bag, but auto otherwise.

[ptomato]

But if the developer just calls round() on its own with no options, that's clearly a bug because it will never do anything.

My point above was that this is true for the other types, but not for Duration.

[justingrant]

Sorry, you're correct-- I didn't understand your point above, but now I do. As you noted there are (unusual) cases where the non-largest unit is unbalanced and the user's goal would be to do balancing on intra-duration units without growing the duration beyond its current largest unit.

AFAIK this is an unusual use case that's much less common than this being a bug where options were unintentionally missing or where the caller expected some default rounding (e.g. to the nearest second) that won't actually happen.

So I'd still support the behavior in the docs where round() and round({}) throw exceptions. This would also be more consistent with other types' round methods where a unit option is required.

[ptomato]

Fair enough, that makes sense.

[cjtenny]

The PR I added makes round() and round({}) throw exceptions; it sounds like that's the consensus, so I'll go ahead and add some tests for them in the morning and ship once approved.