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

Duration format with precision: "second" #1859

Closed
FrankYFTang opened this issue Oct 7, 2021 · 4 comments
Closed

Duration format with precision: "second" #1859

FrankYFTang opened this issue Oct 7, 2021 · 4 comments

Comments

@FrankYFTang
Copy link
Contributor

I think I find a bug when I try to fix my v8 implementaiton of broken test
built-ins/Temporal/Duration/prototype/toString/smallestunit-valid-units.js

what should be the result of
const duration = new Temporal.Duration(1, 2, 3, 4, 5, 6, 7, 987, 654, 321);
duration.toString({ smallestUnit: "second" })

The test said it should be "P1Y2M3W4DT5H6M7S"
but my implementation output "P1Y2M3W4DT5H6M7.000000000S"

Here is why, in

7.3.22 Temporal.Duration.prototype.toString ( [ options ] )

after

4. Let precision be ? ToSecondsStringPrecision(options).

precision is { [[Precision]]: 0, [[Unit]]: "second", [[Increment]]: 1 }.

and then

Return ! TemporalDurationToString(result.[[Years]], result.[[Months]], result.[[Weeks]], result.[[Days]], result.[[Hours]], result.[[Minutes]], result.[[Seconds]], result.[[Milliseconds]], result.[[Microseconds]], result.[[Nanoseconds]], precision.[[Precision]]).

now the values of the last 5 args are

result.[[Seconds]] = 7
result.[[Milliseconds]] = 0
result.[[Microseconds]] = 0 
result.[[Nanoseconds]] = 0
precision.[[Precision]] = 0

Now, in https://tc39.es/proposal-temporal/#sec-temporal-temporaldurationtostring

  1. If any of seconds, milliseconds, microseconds, and nanoseconds are not 0; or years, months, weeks, days, hours, and minutes are all 0, then

since seconds is 7, it will get into the block:

after

a. Let fraction be abs(milliseconds) × 106 + abs(microseconds) × 103 + abs(nanoseconds).
b. Let decimalPart be fraction formatted as a nine-digit decimal number, padded to the left with zeroes if necessary.

so fraction is 0 and decimalPart is "000000000"

c. If precision is "auto", then
i. Set decimalPart to the longest possible substring of decimalPart starting at position 0 and not ending with the code unit 0x0030 (DIGIT ZERO).
d. Else if precision ≠ 0, then
  i. Set decimalPart to the substring of decimalPart from 0 to precision.

Since precision is 0, it will not get into either c-i nor d-i and therefore decimalPart is still "000000000"

e. Let secondsPart be abs(seconds) formatted as a decimal number.

now secondsPart = "7"

f. If decimalPart is not "", then
i. Set secondsPart to the string-concatenation of secondsPart, the code unit 0x002E (FULL STOP), and decimalPart.

since decimalPart is "000000000" it will get into f-i block and secondsPart will now = "7.000000000"

I think the problem is we need to change d to

d. Else if precision = 0, then
  i. Set decimalPart to "".
e. Else
  i. Set decimalPart to the substring of decimalPart from 0 to precision.
@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Oct 7, 2021

@FrankYFTang
Copy link
Contributor Author

Similar issue with test/test262/data/test/built-ins/Temporal/Duration/prototype/toString/blank-duration-precision.js
const blank = new Temporal.Duration();
blank.toString({ fractionalSecondDigits: "auto" })

Following the current spec text will output "PT0.000000000S" not "PT0S" for the same reason

@FrankYFTang
Copy link
Contributor Author

Also test/built-ins/Temporal/Duration/prototype/toString/roundingmode-undefined.js
const duration = new Temporal.Duration(0, 0, 0, 0, 12, 34, 56, 123, 987, 500);

duration.toString({ smallestUnit: "second", roundingMode: undefined })
and
duration.toString({ smallestUnit: "second" })
by the current spec text will both produce "PT12H34M56.000000000S" but not "PT12H34M56S"

@ptomato
Copy link
Collaborator

ptomato commented May 10, 2022

This was closed by #1860.

@ptomato ptomato closed this as completed May 10, 2022
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

No branches or pull requests

2 participants