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.

Sign up for GitHub

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

Editorial: Fix spec link for and remove unused parameter from new ValidateDurationUnitStyle AO #969

Closed
wants to merge 2 commits into from
Closed

Editorial: Fix spec link for and remove unused parameter from new ValidateDurationUnitStyle AO #969

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd1a215
Editorial: Fix spec link for new ValidateDurationUnitStyle AO
trflynn89 Mar 3, 2025
233b4f8
Editorial: Remove unused `unit` parameter from ValidateDurationUnitStyle
trflynn89 Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions spec/durationformat.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ <h1>
1. Set _displayDefault_ to *"auto"*.
1. Let _displayField_ be the string-concatenation of _unit_ and *"Display"*.
1. Let _display_ be ? GetOption(_options_, _displayField_, ~string~, « *"auto"*, *"always"* », _displayDefault_).
1. Perform ? ValidateDurationUnitStyle(_unit_, _style_, _display_, _prevStyle_).
1. Perform ? ValidateDurationUnitStyle(_style_, _display_, _prevStyle_).
1. If _unit_ is *"hours"* and _twoDigitHours_ is *true*, set _style_ to *"2-digit"*.
1. If _unit_ is *"minutes"* or *"seconds"* and _prevStyle_ is *"numeric"* or *"2-digit"*, set _style_ to *"2-digit"*.
1. Return the Record {
Expand All @@ -648,10 +648,9 @@ <h1>
}.
</emu-alg>

<emu-clause id="sec-stringpaddingbuiltinsimpl" type="abstract operation">
<emu-clause id="sec-validatedurationunitstyle" type="abstract operation">
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @trflynn89; I've merged this fix as #970.

<h1>
ValidateDurationUnitStyle (
_unit_: a String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Inclusion of a seemingly-irrelevant unit parameter was intentional; I expect its contents to appear in the message of any thrown exception just as it would have before being factored out of GetDurationUnitOptions in 20daf26. But this is obviously too subtle... perhaps, rather than dropping the parameter, we should have a note explaining how it might be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah gotcha, yeah a note here would be helpful for clarity. In Ladybird's implementation of these changes, I've actually included a couple of extra parameters to this AO to create useful exception messages, so I'm not sure that unit being treated specially by the spec is all that necessary for implementers.

Copy link
Contributor

Choose a reason for hiding this comment

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

#975

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll go ahead and close this PR

_style_: a String,
_display_: a String,
_prevStyle_: a String,
Expand Down