Polyfill: Fix two tests related to date/time formatting#3256
Polyfill: Fix two tests related to date/time formatting#3256
Conversation
ptomato
left a comment
There was a problem hiding this comment.
I dug into this a bit today and found that it requires a bit more refactoring.
You can see in the CI log that intl402/DateTimeFormat/prototype/format/temporal-plaintime-formatting-datetime-style.js unexpectedly fails here. I initially thought this test was incorrect, but by my reading of the spec it is actually correct — dateStyle and timeStyle together should throw when passed to the toLocaleString method of PlainDate, PlainTime, PlainYearMonth, or PlainMonthDay, but should be accepted in the Intl.DateTimeFormat constructor.
I've kind of half-done the refactoring already while trying to figure out what was going on. Let me know if you want to go over it together so you can take it over to finish this PR, or alternatively I can clean up what I have and push it to this PR.
Either way, we'll also need to update the pinned test262 commit to include Anba's modifications to the tests.
|
tc39/test262#4880 should add more test coverage relevant to this. |
Remove 'timeZoneName' from hasAnyDateTimeOptions. This matches the spec (https://tc39.es/proposal-temporal/#sec-getdatetimeformat step 14). hasDateOptions and hasTimeOptions already check for dateStyle and timeStyle respectively, so those can be deleted as well.
This fixes a polyfill bug that was previously not covered by tests. When calling the toLocaleString methods of PlainDate, PlainMonthDay, PlainTime, or PlainYearMonth, the formatter should be created using the 'required' parameter, so that at least one date or time option is required (depending on the type being formatted). This parameter does not correspond to any constructor argument of Intl.DateTimeFormat, so we need to export a CreateDateTimeFormat() function from intl.mjs to expose the functionality to other files. Rename the existing createDateTimeFormat() to internalCreateDateTimeFormat().
e587bcb to
77c14d3
Compare
|
OK, I cleaned up my half-done refactor and pushed it to this branch. I'd appreciate if someone else besides me could take a look at it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3256 +/- ##
==========================================
- Coverage 97.91% 97.89% -0.03%
==========================================
Files 22 22
Lines 10367 10405 +38
Branches 1805 1819 +14
==========================================
+ Hits 10151 10186 +35
- Misses 197 200 +3
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
( https://tc39.es/proposal-temporal/#sec-createdatetimeformat step 45c).
This allows the following tests to pass: