tc39 / proposal-intl-duration-format

https://tc39.es/proposal-intl-duration-format
MIT License
165 stars 18 forks source link

Normative: Make ToDurationRecord same order as ToTemporalPartialDurationRecord #117

Closed FrankYFTang closed 1 year ago

FrankYFTang commented 2 years ago

Currently, ToDurationRecord access the object with the following order: "years", "months", "weeks", "days", "hours", "minutes", "seconds", "milliseconds", "microseconds", "nanoseconds"

This is different from ToTemporalPartialDurationRecord which follow the order of: "days", "hours", "microseconds", "milliseconds", "minutes", "months", "nanoseconds", "seconds", "weeks", "years"

This PR change the accessing order of ToDurationRecord to be the same as ToTemporalPartialDurationRecord without impacting the math and order of other 4 AOs.

Please compare https://tc39.es/proposal-temporal/#sec-temporal-duration-records https://tc39.es/proposal-temporal/#sec-temporal-totemporalpartialdurationrecord

against https://tc39.es/proposal-intl-duration-format/#table-duration-components https://tc39.es/proposal-intl-duration-format/#sec-todurationrecord

Notice three other AOs in Intl.DurationFormat CANNOT change the order to keep the math

DurationRecordSign / PartitionDurationFormatPattern and Intl.DurationFormat

FrankYFTang commented 2 years ago

ToDurationRecord should probably just be ToTemporalDurationRecord at some point, but this alignment is definitely an improvement.

When I first prototype, I call into my implementation of ToTemporalDurationRecord in my v8 code. But it actually is not compatable with the current Intl.DurationFormat spec text and break the test https://github.com/tc39/test262/blob/main/test/intl402/DurationFormat/prototype/format/invalid-arguments-throws.js

somehow the test (based on https://tc39.es/proposal-intl-duration-format/#sec-todurationrecord ) will throw RangeError on those if I reuse the code of ToTemporalDurationRecord because they are not Object and after the ToString convert them to string and pass to the parser the do not meet the grammar. But the spec is mandating us to throw TypeError now, which is closer to ToTemporalPartialDurationRecord instead of ToTemporalDurationRecord. We should either change the spec to make it closer to ToTemporalDurationRecord now or decide we should make it closer to ToTemporalPartialDurationRecord instead.

sffc commented 2 years ago

Discussion: https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-09-01.md#normative-make-todurationrecord-same-order-as-totemporalpartialdurationrecord-117

Conclusion: Discuss with Temporal first so we don't duplicate work.

FrankYFTang commented 2 years ago

@ptomato @gibson042 please help to resolve the need of this PR.

ptomato commented 2 years ago

This looks in line with https://github.com/tc39/proposal-temporal/issues/2254 except that we want to enforce the alphabetical order by sorting the fields, rather than having it be implicit due to the table order. However, we have not yet made that change in Temporal so it seems fine to make this change now to bring the operation in line with Temporal, and later make a change to sort the fields.

Is there any other input you need from me?

FrankYFTang commented 2 years ago

Is there any other input you need from me?

@ptomato

yes, this PR is currently blocked by the comment in https://github.com/tc39/ecma402/blob/master/meetings/notes-2022-09-01.md#normative-make-todurationrecord-same-order-as-totemporalpartialdurationrecord-117 "SFC: If we convince Temporal to switch, we won’t need a second table. We should talk to the champions in order to see if we can make them change the order."

SFC's "to switch" means "to switch [to sort from the order in the Table https://tc39.es/proposal-temporal/#table-temporal-duration-record-fields to the order in the Table https://tc39.es/proposal-intl-duration-format/#table-duration-components "

My undrstanding is Temporal champions are not not willing to do such switch so I propose this PR. But SFC believe it is still possible someone (surely not me, but I have no idea who that will be) could still possible "convince Temporal to switch" threfore "won’t need a second table." So he want to "talk to the champions in order to see if we can make them change the order."

Please answer SFC a clear answer- either a YES, Temporal champions are willing to switch the order to the same as https://tc39.es/proposal-intl-duration-format/#table-duration-components OR NO Temporal champions are NOT willing to switch the order to the same as https://tc39.es/proposal-intl-duration-format/#table-duration-components

ptomato commented 2 years ago

Sorry, I thought https://github.com/tc39/proposal-temporal/issues/2286#issuecomment-1248687027 and https://github.com/tc39/proposal-temporal/issues/2254#issuecomment-1248693340 already gave a clear answer on that. The answer is no, we are not going to read the properties in magnitude order for Duration. They should be read in alphabetical order. Shane was present at that discussion and agreed with the conclusion.

However, once we make the change to alphabetize the order explicitly instead of depending implicitly on the table order, we can order the rows of the table any way that is convenient for DurationFormat.

ryzokuken commented 1 year ago

Superceded by #129