ecma402 icon indicating copy to clipboard operation
ecma402 copied to clipboard

Published 20 hours ago • verified publisher icon tc39

Editorial: Move NumberFormat objects to DateTimeFormat constructor an…

Open longlho opened this issue 4 years ago • 15 comments
trafficstars

…d add new internal slots

fix #563

longlho avatar Apr 26 '21 03:04 longlho

@ryzokuken done. Thanks a lot for the review!

longlho avatar Apr 26 '21 13:04 longlho

CC @FrankYFTang -- can you review this proposed change from @longlho and comment on whether it would impact implementations like V8?

sffc avatar Apr 27 '21 05:04 sffc

@sffc I think @anba mentioned that it wouldn't affect ICU-based implementations.

ryzokuken avatar Apr 27 '21 05:04 ryzokuken

CC @FrankYFTang -- can you review this proposed change from @longlho and comment on whether it would impact implementations like V8?

I believe this is editorial and has no implementation impact to v8.

FrankYFTang avatar Apr 27 '21 06:04 FrankYFTang

@gibson042 @leobalter can we get one editor +1 and merge this? :D

ryzokuken avatar Apr 27 '21 13:04 ryzokuken

@anba could you also take a look at this?

FrankYFTang avatar May 20 '21 18:05 FrankYFTang

@anba care for another look? thanks!

longlho avatar May 24 '21 14:05 longlho

Ping @anba PTAL.

ryzokuken avatar Jun 07 '21 09:06 ryzokuken

@ryzokuken any update on this? ping @anba as well

longlho avatar Aug 17 '21 00:08 longlho

@longlho I'd been waiting for @anba to give their approval before merging but I suppose I could go through their comments later today and see if their concerns have been addressed and merge if they are busy right now.

ryzokuken avatar Aug 17 '21 07:08 ryzokuken

thanks a lot for the fixes @gibson042 !

longlho avatar Aug 18 '21 00:08 longlho

gentle ping @anba :)

longlho avatar Sep 04 '21 17:09 longlho

ping @ryzokuken

longlho avatar Sep 29 '21 04:09 longlho

@ryzokuken do you know if this is still mergeable? happy to rebase if that's the case

longlho avatar Mar 29 '22 02:03 longlho

Sorry for the delay, @longlho. It looks all good from my side, but looks like Andre still couldn't review it. If you rebase it, perhaps we could merge it and file an issue to address their concerns in a follow-up?

ryzokuken avatar Apr 27 '22 14:04 ryzokuken

@anba @longlho @ryzokuken Who needs to take the next action on this PR?

sffc avatar Aug 23 '23 23:08 sffc

Hmm, this change may no longer be possible, because the current Temporal draft spec changes Intl.DateTimeFormat to be able to format Temporal objects with different resolved patterns. For example when formatting Temporal.PlainTime and Temporal.PlainDateTime with a single Intl.DateTimeFormat, the resolved pattern for Temporal.PlainTime can have a different amount of fractionalSecondDigits when compared to the resolved pattern for Temporal.PlainDateTime. Therefore it's not possible to store a single [[FractionalSecondNumberFormat]] internal slot.

anba avatar Aug 24 '23 06:08 anba

Oh I see, yeah, I don't think we should do this. The spec does not forbid implementations from doing internal optimizations like this so long as the observable behavior is the same.

I'll close the PR since it has been idle for a long time and it's not clear if it is a good PR based on this and @anba's comment.

sffc avatar Aug 24 '23 21:08 sffc