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

hours: "numeric" w/ 0 in minutes and non 0 in seconds cause strange output. #170

Closed
FrankYFTang opened this issue Aug 25, 2023 · 6 comments
Assignees
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion

Comments

@FrankYFTang
Copy link
Collaborator

@anba's tc39/test262#3903 surface the following issue
intl402/DurationFormat/prototype/format/numeric-hour-with-zero-minutes-and-non-zero-seconds.js

What should be the output of

 (new Intl.DurationFormat("en", {  hours: "numeric",})).format({  hours: 1,  minutes: 0,  seconds: 3})

@anba's code, following the spec text produce the expectation as

1, 03

and currently, the buggy v8 implement 1:03 (which is neither correct)
I think it should be
1:00:03
v8 also incorrectly output

 (new Intl.DurationFormat("en", {  hours: "numeric",})).formatToParts({  hours: 1,  minutes: 0,  seconds: 3})
[{type: "integer", value: "1", unit: "hour"}, {type: "literal", value: ":"}, {type: "integer", value: "03", unit: "second"}]

I think it should be

[{type: "integer", value: "1", unit: "hour"}, {type: "literal", value: ":"}, {type: "integer", value: "00", unit: "minute"}, {type: "literal", value: ":"}, {type: "integer", value: "03", unit: "second"}]

but that is not following the spec text.

We should also consider case like

 (new Intl.DurationFormat("en", {  hours: "numeric", fractionalDigits: 9 })).format({  hours: 1,   microseconds: 4})
 (new Intl.DurationFormat("en", {  hours: "numeric", fractionalDigits: 9})).format({  hours: 1,   milliseconds: 5})
 (new Intl.DurationFormat("en", {  hours: "numeric", fractionalDigits: 9})).format({  hours: 1,   nanoseconds: 6})

@FrankYFTang
Copy link
Collaborator Author

@FrankYFTang
Copy link
Collaborator Author

I think the problem is here

9. If unit is "hours" or "minutes", then
  a. If unit is "hours", then
    i. Let nextValue be duration.[[Minutes]].
    ii. Let nextDisplay be durationFormat.[[MinutesDisplay]].
  b. Else,
    i. Let nextValue be duration.[[Seconds]] + duration.[[Milliseconds]] / 103 + duration.[[Microseconds]] / 106 + duration.[[Nanoseconds]] / 109.
    ii. Let nextDisplay be durationFormat.[[SecondsDisplay]].
  c. If nextValue is not 0 or nextDisplay is not "auto", then
    i add  separator block...

We actually do not need to calculate nextValue since the only usage is compare against 0, we just need a boolean for step c and set that boolean in sub steps of a and b

How about

9. If unit is "hours" or "minutes", then
a. Let addSep be *false*.
b If duration.[[Nanoseconds]] is not 0, set addSep to *true*.
c Else if duration.[[Microseconds]] is not 0, set addSep to *true*.
d Else if duration.[[Milliseconds]] is not 0, set addSep to *true*.
e Else if duration.[[Seconds]] is not 0, set addSep to *true*.
f Else if unit is "hours" and duration.[[Minutes]] is not 0, set addSep to *true*.
g Else if unit is "hours" and durationFormat.[[MinutesDisplay]] is "always", set addSep to *true*.
h Else if unit is "minutes" and durationFormat.[[SecondsDisplay]] is "always", set addSep to *true*.
i If addSep is *true*, then
  i add  separator block...

@anba
Copy link
Contributor

anba commented Aug 25, 2023

The proposed changes in #170 (comment) will add a separator even when the next unit isn't displayed.

If we want to return "1:00:03" for the above mentioned test case, it's also necessary to make some adjustments to current step 4.l:

If value is not 0 or display is not "auto", then

And change it to:

If value is not 0 or display is not "auto" or separated is true, then

And then remove step 4.l.ii.5:

  1. Else,
    a. Set separated to false.

If #166 gets accepted, slightly different changes are necessary.

@sffc
Copy link
Collaborator

sffc commented Sep 18, 2023

My understanding is that new Intl.DurationFormat("en", { hours: "numeric" }) is supposed to imply that the remainder of fields are numeric and should therefore output the expected results, but it is definitely a problem that minutes are not displayed when zero. Hmm

@sffc
Copy link
Collaborator

sffc commented Sep 18, 2023

@ben-allen to discuss with @romulocintra and @ryzokuken on who should work on this.

@ben-allen
Copy link
Collaborator

Marking closed as fixed by #180 & #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion
Projects
None yet
Development

No branches or pull requests

4 participants