-
Notifications
You must be signed in to change notification settings - Fork 472
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
DurationFormat: Add tests for durations with style: "digital"
, hoursDisplay: "auto"
and zero hours
#4367
Conversation
// basic zero hours | ||
[{ hours: 0, minutes: 0, seconds: 2 }, "00:02"], | ||
[{ hours: 0, minutes: 1, seconds: 2 }, "01:02"], | ||
[{ days: 1, hours: 0, minutes: 1, seconds: 2 }, "1 day, 01:02"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chrome prints "1 day:01:02"
for this, which is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a V8 bug.
In FormatNumericUnits, steps 10-11 will set hoursFormatted
to false
, because the hour value is zero and the hours display is not "always"
. Step 17 passes hoursFormatted
to FormatNumericMinutes. FormatNumericMinutes, step 2 only appends the separator if hoursDisplayed
is false
. And the value of hoursDisplayed
is hoursFormatted
from FormatNumericUnits. That means no separator should be appended for this case.
esvu on CI fails but probably this PR isn't related
|
...l402/DurationFormat/prototype/format/digital-style-with-hours-display-auto-with-zero-hour.js
Outdated
Show resolved
Hide resolved
// basic zero hours | ||
[{ hours: 0, minutes: 0, seconds: 2 }, "00:02"], | ||
[{ hours: 0, minutes: 1, seconds: 2 }, "01:02"], | ||
[{ days: 1, hours: 0, minutes: 1, seconds: 2 }, "1 day, 01:02"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a V8 bug.
In FormatNumericUnits, steps 10-11 will set hoursFormatted
to false
, because the hour value is zero and the hours display is not "always"
. Step 17 passes hoursFormatted
to FormatNumericMinutes. FormatNumericMinutes, step 2 only appends the separator if hoursDisplayed
is false
. And the value of hoursDisplayed
is hoursFormatted
from FormatNumericUnits. That means no separator should be appended for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to defer to anba here
@anba Thank you for review! |
@Ms2ger Anba's suggestions have been applied, probably we can merge this |
…rsDisplay: "auto"` and zero hours
…h-hours-display-auto-with-zero-hour.js Co-authored-by: André Bargull <[email protected]>
29a1581
to
eac60e2
Compare
The implementation of
Intl.DurationFormat
in WebKit (JavaScriptCore) has passed all test262 test cases, but we discovered two incompatibilities with the specification. This Pull Request adds tests addressing those issues.The first one is described in bug 285078. A separator is printed erroneously for an unprinted hours value:
The second one is described in bug 285212. A negative sign is not printed:
Both issues occur when
style: "digital"
,hoursDisplay: "auto"
, and the hours value is 0 or absent. For that reason, they are combined into a single test file.