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

test formatting when microseconds or nanoseconds are the first numeric-styled unit #4034

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

ben-allen
Copy link
Contributor

If microseconds are the first numeric-styled unit, they should be displayed as a fractional component of milliseconds. Likewise, if nanoseconds is the first numeric-styled unit, they should be displayed as a fractional component of microseconds.

@ben-allen ben-allen requested a review from a team as a code owner March 28, 2024 16:39
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get all the way through this one, but I have a couple of questions:

  1. If I'm understanding correctly, GetDurationUnitOptions step 4 basically forces {microseconds: 'numeric'} and {nanoseconds: 'numeric'} to {microseconds: 'fractional'} and {nanoseconds: 'fractional'} respectively. Do we need test coverage that these things produce the same output?
  2. .format(d.microseconds.toString() + decimalSeparator + d.nanoseconds.toString())) seems OK for this particular input but it might be better to add .padStart(3, '0') to be robust against someone editing the values in d.

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished going through it, and I don't have any additional comments besides what I wrote the other day.

@ben-allen ben-allen force-pushed the df-fractions-of-subseconds branch from 5947d1e to b12d693 Compare April 11, 2024 15:34
@ben-allen
Copy link
Contributor Author

Added the padding for the fractional components -- good catch!

With regard to testing for "fractional" style: The "fractional" style is never directly used by users, who only have "numeric" available, and so we don't need to have tests for it. The internal-use-only "fractional" style exists for a couple of different reasons:

  1. The "numeric" style means two dramatically different things, depending on the unit to which it's applied. If the unit is hours, minutes, or seconds, it means something like "format this as on a digital clock", when it's a subsecond unit it means "make this a fractional component of the previously displayed unit". Using the name "fractional" simplifies the logic for formatting numeric units, particularly once Editorial: Refactor handling of numeric-like styles, support differing hours/minutes vs minutes/seconds separators proposal-intl-duration-format#188 lands
  2. A hypothetical DFv2 could allow users to style durations with fractional components of minutes or hours as well as seconds, milliseconds, and microseconds. In this case making "fractional" an actual user-specifiable style rather than just something for internal use would be necessary.

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. fractional not being a user-facing option was what I was missing!

@ptomato ptomato force-pushed the df-fractions-of-subseconds branch from b12d693 to 7a83352 Compare April 11, 2024 23:57
@ptomato ptomato merged commit 046dff4 into tc39:main Apr 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants