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

Should 'second' be the smallest unit accepted? #64

Open
justingrant opened this issue Jun 3, 2021 · 29 comments
Open

Should 'second' be the smallest unit accepted? #64

justingrant opened this issue Jun 3, 2021 · 29 comments
Assignees
Labels

Comments

@justingrant
Copy link

Should 'second' be the smallest unit accepted? If someone wants to hide fractions, then fractionalSecondDigits can be used.

From #32 (comment)

@sffc
Copy link
Collaborator

sffc commented Jun 4, 2021

Hmm.

I think this makes sense. If seconds are enabled, what should be the default behavior when the incoming duration has milliseconds, microseconds, or nanoseconds? My initial thought would be that we should display as many digits as necessary without trailing zeros. So, for example,

new Intl.DurationFormat("en").format({ seconds: 1, milliseconds: 120 });
// "1.12 seconds"

And if fractionalSecondDigits is specified, then it is like setting both minimumFractionDigits and maximumFractionDigits in Intl.NumberFormat.

new Intl.DurationFormat("en", { fractionalSecondDigits: 2 }).format({ seconds: 1, milliseconds: 101 });
// "1.10 seconds"

@ryzokuken
Copy link
Member

ryzokuken commented Jun 8, 2021

Does this correspond to the locale data? I don't think so, I thought the locale data did:

new Intl.DurationFormat("en").format({ seconds: 1, milliseconds: 120 });
// "1 seconds and 120 milliseconds"

@sffc
Copy link
Collaborator

sffc commented Jun 8, 2021

Oh, good point. Yeah, both forms (fraction digits and separate fields) are supported by locale data, since this is just a matter of interpolating units in a list. Perhaps we should just make this a toggle switch of some sort in the options bag.

Thanks @justingrant and @ryzokuken for raising this issue! It had flown under my radar.

@justingrant
Copy link
Author

I'm not sure there's a strong use case for spelling out ms/μs/ns units. Seems like we could simply use decimal seconds in all cases.

@sffc
Copy link
Collaborator

sffc commented Jun 9, 2021

I'm not sure there's a strong use case for spelling out ms/μs/ns units. Seems like we could simply use decimal seconds in all cases.

Lots of use cases.

  • Benchmarking of computer programs
  • Half-life of radioactive compounds
  • Cycle time (inverse frequency) for light and sound waves

Just look on the Wikipedia pages for milliseconds, microseconds, and nanoseconds.

@justingrant
Copy link
Author

Ahh, you're right. We should support single-unit cases like ${x} milliseconds. The particular case I'm concerned about is 14 seconds and 200 milliseconds. Seems like 99% of the time (maybe 100% of the time!) callers will want 14.2 seconds instead of spelling out a base-10 fractional amount. You can probably make the same argument about smaller units too, e.g. 243.45 microseconds. How should this API make the vastly-most-common case the default, and do we need to support the rare case at all?

@sffc
Copy link
Collaborator

sffc commented Jun 9, 2021

How about if subsecond digits are appended to the smallest unit on the constructor list. For example,

new Intl.DurationFormat("en", {
    units: ["second"]
}).format({
    second: 5,
    millisecond: 120
});
// 5.12 seconds

new Intl.DurationFormat("en", {
    units: ["second", "millisecond"]
}).format({
    second: 5,
    millisecond: 120
});
// 5 seconds, 120 milliseconds

new Intl.DurationFormat("en", {
    units: ["millisecond"]
}).format({
    millisecond: 5,
    microsecond: 120
});
// 5.12 milliseconds

Seems like 99% of the time (maybe 100% of the time!) callers will want 14.2 seconds instead of spelling out a base-10 fractional amount.

This is an overly broad statement because (1) I can't speak for what all users in all languages expect in their own languages, and (2) even in English I can think of some cases where one may want the expanded form, such as when milliseconds are the "main" unit but one must display overflow to higher order units.

@justingrant
Copy link
Author

justingrant commented Jun 9, 2021

OK, sounds like you're proposing this:

  • all units are accepted as constructor options
  • if the smallest unit option in the constructor is...
    • ...'second' or larger (see note below re: "or larger"), then any smaller non-zero units will be displayed as fractional seconds, e.g. "1.123456789 seconds"
    • ...'millisecond' , then any smaller non-zero units in the duration will be displayed as fractional decimal milliseconds, e.g. "1.123456 milliseconds"
    • ...'microsecond' , then any smaller non-zero units in the duration will be displayed as fractional decimal microseconds, e.g. "1.123 microseconds"
  • The fractionalDigits (needs bikeshed-- see below) option controls how many decimal digits are shown after the decimal point of the smallest localized unit in the output.
    • If this option is 'auto' or is omitted, then trailing zeroes are omitted
    • If the smallest unit option in the constructor is 'second' or larger (or is omitted), then 0-9 number values are accepted for this option
    • If the smallest unit option in the constructor is 'millisecond' then 0-6 is accepted
    • If the smallest unit option in the constructor is 'microsecond' then 0-3 is accepted
    • If the smallest unit option in the constructor is 'nanosecond' then only 0 is accepted

Is this correct? Revise if not!

Issues / Questions:

  • If the smallest unit option in the constructor is larger than 'second' then the outcome is may be dependent on the discussion in Options for smallestUnit/largestUnit and hideZeroValued #32. If we throw for unexpected units, then would we still throw if the unexpected units were smaller than 'second'? What if the smallest unit option is 'minute' and the duration had non-zero milliseconds; would we still output "0.123 seconds" in that case? If the outcome of Options for smallestUnit/largestUnit and hideZeroValued #32 is "best effort" then I think this is a non-issue; we'd show "0.123 seconds" in that case.
  • What should we do if the duration has non-zero data smaller than fractionalDigits? Should we truncate? Round? Offer a rounding option?
  • What should be the name of the option for displaying fractional digits? In Temporal toString methods it's fractionalSecondDigits but here the fractions can be displayed for milliseconds or microseconds so that name won't work. And if we change the name here, should we rename the Temporal one, and if so what should it be renamed to? My assumption is that we shouldn't rename the Temporal one.

@sffc
Copy link
Collaborator

sffc commented Jun 9, 2021

Yep, that sounds right!

Agreed about this being dependent on #32.

I would truncate the extra digits, only because rounding gets complicated. How would you round 59.999 seconds to 2 fraction digits? We don't want to get into the messiness of shifting data around between fields; that should be done in client code before Intl.DurationFormat gets the duration.

fractionalDigits or just fractionDigits seems fine. Maybe fractionDigits since it is closer to Intl.NumberFormat's minimumFractionDigits and maximumFractionDigits.

Another question:

  • What if milliseconds, microseconds, or nanoseconds are greater than 999?

@justingrant
Copy link
Author

Hmm, >999 is a tough problem! Some possible solutions: (probably incomplete list)

  1. throw in any cases where overflow would happen in the decimal portion
  2. apply overflow to the next-largest unit, stopping at seconds. If the largest unit option is larger than 'second' then throw if the overflow results in >59 seconds.
  3. apply all overflow to the next-largest unit, stopping at hours. If the largest unit option is larger than 'hour' then throw if the overflow results in >24 hours.

This is also dependent on #32, but in the other direction than my previous comment. In this case, the "exception" resolution to #32 is the easiest way to solve the >999 problem because unexpected values cause exceptions in all cases. If choice is "best effort" then >999 behavior is not obvious.

@sffc
Copy link
Collaborator

sffc commented Jun 9, 2021

If we #32 gets resolved as "best effort" then I'd probably say to go with the 2nd option listed there (carry over to larger fields, stopping at seconds). This makes sense since we would already be equating seconds and smaller as things where digits can be transferred. So, the algorithm would basically be,

If the smallest unit is "second" or greater, let secondsToDisplay be seconds + milliseconds*10^-3 + microseconds*10^-6 + nanoseconds*10^-9.
Else if the smallest unit is "millisecond", let millisecondsToDisplay be milliseconds + microseconds*10^-3 + nanoseconds*10^-6.
Else if the smallest unit is "microsecond", let microsecondsToDisplay be microseconds + nanoseconds * 10^-3.
Else, let nanosecondsToDisplay be nanoseconds.

I don't care too much about this case since it is an edge case; we just need to make sure we have well-defined behavior.

@sffc
Copy link
Collaborator

sffc commented Jun 9, 2021

However, I would not throw an exception if we happen to exceed 60 seconds. For example, I think the following is totally a valid thing to do.

const durf = new Intl.DurationFormat("en", { units: ["second"] });
durf.format({ milliseconds: 123456 });
// "123.456 seconds"

Even if it weren't valid, if we resolve to best effort on #32, I would like to also do a best effort here and not throw the exception.

@justingrant
Copy link
Author

However, I would not throw an exception if we happen to exceed 60 seconds.

The single-unit case is easy. The multiple-unit case is more interesting. Do you mean that you'd expect the following output if we chose "best effort" ?

const durf = new Intl.DurationFormat("en", { units: ["second"] });
durf.format({ minutes: 30, milliseconds: 123456 });
// "30 minutes and 123.456 seconds"

@sffc
Copy link
Collaborator

sffc commented Jun 10, 2021

Yes, I think that output is fine. The idea with best effort is that you always present something that makes sense to the user. In this case, garbage in, garbage out.

@justingrant
Copy link
Author

I'd tentatively support what you're recommending above. This feels complicated enough that there may be edge cases we haven't thought of, so probably needs a few more eyeballs to review to make sure we're not missing anything important.

@sffc
Copy link
Collaborator

sffc commented Aug 3, 2021

With the new decisions in #32, the decisions here mostly stand, with the following changes:

  • millisecond, microsecond, and nanosecond default to display "auto", and display "auto" means that data in those fields is merged into the next larger field
  • display "always" will cause that unit, and all units greater than it, to be displayed as text, rather than being merged

Examples:

# Duration Options Result
1 { milliseconds: 500 } {} "0.5 sec"
2 { milliseconds: 500 } { fractionalDigits: 2 } "0.50 sec"
3 { milliseconds: 500 } { millisecondDisplay: "always" } "500 ms"
4 { milliseconds: 500 } { millisecondDisplay: "always",
fractionalDigits: 2 }
"500.00 ms"
5 { microseconds: 500 } { millisecondDisplay: "always" } "0.5 ms"
6 { microseconds: 500 } { microsecondDisplay: "always" } "500 μs"

Some edge cases might include:

# Duration Options Result
7 { microseconds: 5500 } { microsecondDisplay: "always" } "5,500 μs"
8 { milliseconds: 5,
microseconds: 500 }
{ microsecondDisplay: "always" } "5 ms 500 μs"
9 { seconds: 5,
microseconds: 500 }
{ microsecondDisplay: "always" } "5 sec 500 μs"
10 { seconds: 5 } { microsecondDisplay: "always" } "5 sec 0 μs"

@sffc
Copy link
Collaborator

sffc commented Aug 4, 2021

I thought of another way to express this.

Instead of using Display "auto", we could use Style "numeric". If users want to disable the fractional display, they can set the style appropriately.

For example, { millisecond: "short" } triggers the millisecond field to be displayed as its own field.

@justingrant
Copy link
Author

Instead of using Display "auto", we could use Style "numeric". If users want to disable the fractional display, they can set the style appropriately.

If we go with this suggestion, what are the ambient default styles if no style is specified? Would seconds be 'short' but smaller units be 'numeric' ?

Also, what would be the difference in behavior between 'always' and 'auto' for those smaller units if 'numeric' were the style for those units?

@sffc
Copy link
Collaborator

sffc commented Aug 4, 2021

If we go with this suggestion, what are the ambient default styles if no style is specified? Would seconds be 'short' but smaller units be 'numeric' ?

The ambient default styles for [milli/micro/nano]seconds would always be "numeric" even if the style option is long, short, narrow, or digital.

Also, what would be the difference in behavior between 'always' and 'auto' for those smaller units if 'numeric' were the style for those units?

hmm. I think "always" paired with "numeric" does not make sense for these units. Perhaps we can throw an exception if display "always" is specified with an incompatible style option.

@sffc
Copy link
Collaborator

sffc commented Aug 4, 2021

One extremely minor edge case: { microseconds: "short", fractionalDigits: 2 }, which is the same as

{
  seconds: "short",
  milliseconds: "numeric",
  microseconds: "short",
  nanoseconds: "numeric",
  fractionalDigits: 2,
}

What happens when you give this PT0.123456789S? Do you get:

  1. "0.123 sec 456.79 μs" (fractionalDigits applies only to the smallest unit)
  2. "0.12 sec 456.79 μs" (fractionalDigits applies to both seconds and microseconds)
  3. "123 ms 456.79 μs" (numeric is dropped if there is a non-numeric unit that follows)

I lean toward 1.

@justingrant
Copy link
Author

justingrant commented Aug 4, 2021

One extremely minor edge case: { microseconds: "short", fractionalDigits: 2 },

Good catch. IMHO, the most likely expected output would be "123 ms 456.79 μs". This seems similar to the case of hours/minutes/seconds where some combination of styles aren't really valid. How did we agree to handle a case like { hours: 'numeric', minutes: 'short', seconds: 'numeric' }?

hmm. I think "always" paired with "numeric" does not make sense for these units. Perhaps we can throw an exception if display "always" is specified with an incompatible style option.

Another possibility is that 'always' implies a default fractionalDigits that shows all three digits of that unit. fractionalDigits: 6 seems closest to the likely user intent of { seconds: 'short', microsecondsDisplay: 'always' } => "12.234500 seconds".

@sffc
Copy link
Collaborator

sffc commented Aug 5, 2021

One extremely minor edge case: { microseconds: "short", fractionalDigits: 2 },

Good catch. IMHO, the most likely expected output would be "123 ms 456.79 μs". This seems similar to the case of hours/minutes/seconds where some combination of styles aren't really valid. How did we agree to handle a case like { hours: 'numeric', minutes: 'short', seconds: 'numeric' }?

Good question. Worth discussing.

In general I think we should try not to allow garbage input when it's easy to determine when there is garbage input. I think for the "numeric" style, we can impose these rules:

  1. "hour", "minute", and "second" must be either all "numeric" or all not "numeric".
    • We could weaken this rule, but maybe it's good to start strict.
  2. If "millisecond" is "numeric", then "microsecond" must also be "numeric".
  3. If "microsecond" is "numeric", then "nanosecond" must also be "numeric".

I think those rules cover all our cases?

If any rule fails, then we throw a TypeError in the constructor.

hmm. I think "always" paired with "numeric" does not make sense for these units. Perhaps we can throw an exception if display "always" is specified with an incompatible style option.

Another possibility is that 'always' implies a default fractionalDigits that shows all three digits of that unit. fractionalDigits: 6 seems closest to the likely user intent of { seconds: 'short', microsecondsDisplay: 'always' } => "12.234500 seconds".

I thought about this, but I'm not a fan, because we should be converging around fractionalDigits as the tool to achieve that purpose. It seems complicated to spec out this behavior when it isn't the intended solution.

@ryzokuken
Copy link
Member

Second is not the smallest unit accepted, but we now allow displaying all sub-second units as fractional seconds, with the added benefit of being able to control the exact number of fractional digits. @justingrant can this be closed?

@ryzokuken
Copy link
Member

Closing

@sffc
Copy link
Collaborator

sffc commented Aug 24, 2022

There is a whole lot of discussion in this thread which appears to be not reflected in the spec. Please make sure that the open ends are tied up regarding the interaction of fractionalSecondDigits and millisecond/microsecond/nanosecond options.

@ryzokuken
Copy link
Member

Clarification, once you go "numeric" somewhere, you cannot go "short" for smaller units.

The API has changed quite a bit since this was discussed, fractionalDigits was put in to overcome most if not all the issues above. I read through the whole thing but cannot realize if there's something left to be addressed here.

@romulocintra
Copy link
Member

Clarification, once you go "numeric" somewhere, you cannot go "short" for smaller units.

The API has changed quite a bit since this was discussed, fractionalDigits was put in to overcome most if not all the issues above. I read through the whole thing but cannot realize if there's something left to be addressed here.

Maybe some additional tests for this would be necessary. cc @ben-allen

@sffc
Copy link
Collaborator

sffc commented Aug 23, 2023

What needs to be done here:

  1. Tests should be added to enforce Should 'second' be the smallest unit accepted? #64 (comment) assuming that is consistent with the spec
  2. It seems the current behavior is to throw a RangeError in the case outlined in Should 'second' be the smallest unit accepted? #64 (comment); make sure there is a test for that

@sffc
Copy link
Collaborator

sffc commented Oct 18, 2023

New update: since we no longer allow display "always" on subsecond fields (#155), there is no action on the first comment linked above.

On the second comment, we should just make sure there is test coverage that long/short/narrow cannot follow any numeric, 2-digit, or fractional style field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants