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

Refactor numeric styles #185

Merged
merged 2 commits into from
Feb 1, 2024
Merged

Conversation

ben-allen
Copy link
Collaborator

Editorial: updated resolvedOptions and Table 3 to use new "fractional" style for subsecond units.

This is introduced to simplify the logic for handling "numeric" units inGetDurationUnitOptions and PartitionDurationFormatPattern, as this style means something different for subsecond units than it does for hours, minutes, and seconds.

This change is non-observable: users still use "numeric" to specify
the style in all cases, and "fractional" is converted back to "numeric" in
resolvedOptions.

Some unrelated wording clarifications: regularized table iteration language, replaced "options bag" with "Object" when describing parameters of GetDurationUnitOptions.

ben-allen and others added 2 commits January 16, 2024 06:19
…" style for subsecond units.

This is introduced to simplify the logic for handling "numeric" units in
GetDurationUnitOptions and PartitionDurationFormatPattern, as this style means
something different for subsecond units than it does for hours, minutes, and
seconds.

This change is non-observable: users still use "numeric" to specify
the style in all cases, and "fractional" is converted back to "numeric" in
resolvedOptions.
Copy link
Collaborator

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Seems good although the outcome is not as concise as I had been hoping it would be. The steps to fix "numeric" vs "fractional" for internal/external uses adds as much code as using "fractional" removes.

@ben-allen
Copy link
Collaborator Author

I have work in progress for a PR to apply on top of this one which factors out handling for times in numeric and numeric-like styles into their own AOs, though less in the interest of reducing LOC and more in the interest of reducing the amount of state (_prevStyle, _nextStyle_) carried around when formatting units with numeric styles.

@ben-allen ben-allen merged commit eec6dd7 into tc39:main Feb 1, 2024
ben-allen added a commit to ben-allen/proposal-intl-duration-format that referenced this pull request Feb 8, 2024
* Editorial: updated resolvedOptions and Table 3 to use new "fractional" style for subsecond units.

This is introduced to simplify the logic for handling "numeric" units in
GetDurationUnitOptions and PartitionDurationFormatPattern, as this style means
something different for subsecond units than it does for hours, minutes, and
seconds.

This change is non-observable: users still use "numeric" to specify
the style in all cases, and "fractional" is converted back to "numeric" in
resolvedOptions.]

Separate PartitionDurationFormatPattern's handling for "numeric" and "2-digit" into their own AOs
@FrankYFTang
Copy link
Collaborator

I believe intl402/DurationFormat/constructor-options-defaults.js in test262 need to be adjust after this PR but I have a hard time to figure out how to hange it. @ben-allen could you change that

@FrankYFTang
Copy link
Collaborator

This change is non-observable: users still use "numeric" to specify the style in all cases, and "fractional" is converted back to "numeric" in resolvedOptions.

Wait. how could this change be non-observable? you change "numeric" to "fractional" in Table 3 and therefore what got passed into GetDurationUnitOptions() for milliseconds, microseconds, and nanoseconds are now « "long", "short", "narrow", "fractional" »
and what got passed into

1. Let style be ? GetOption(options, unit, string, stylesList, undefined).

in stylesList is « "long", "short", "narrow", "fractional" »
and if user pass in {microseconds: "numeric"} it GetOption will throw RangeError.

@ben-allen
Copy link
Collaborator Author

Thanks for the catch! PR to fix it here: #197

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

Successfully merging this pull request may close these issues.

3 participants