Editorial: Fixes for remaining issues from Anba #2995
Conversation
spec/intl.html
Outdated
| @@ -701,17 +701,17 @@ <h1> | |||
| 1. Let _bestFormat_ be DateTimeStyleFormat(_dateStyle_, _timeStyle_, _styles_). | |||
| 1. <ins>If _dateStyle_ is not *undefined*, then</ins> | |||
| 1. <ins>Set _dateTimeFormat_.[[TemporalPlainDateFormat]] to _bestFormat_.</ins> | |||
| 1. <ins>Set _dateTimeFormat_.[[TemporalPlainYearMonthFormat]] to _bestFormat_.</ins> | |||
| 1. <ins>Set _dateTimeFormat_.[[TemporalPlainMonthDayFormat]] to _bestFormat_.</ins> | |||
| 1. <ins>Set _dateTimeFormat_.[[TemporalPlainYearMonthFormat]] to GetDateTimeFormat(_styles_, _formatMatcher_, _bestFormat_, ~year-month~, ~year-month~, ~relevant~).</ins> | |||
There was a problem hiding this comment.
styles is a DateTime Styles Record, but GetDateTimeFormat requires a List of DateTime Format Records.
For context, these are the relevant CLDR patterns: https://github.com/unicode-org/cldr/blob/8fdb9a1ba925ea7dee6669fb11f2bf3a9077d34c/common/main/en.xml#L2573-L2628
The <datetimeSkeleton> element is relatively new, earlier CLDR versions only had <pattern>.
So implementation-wise it's maybe possible to map the pattern to a skeleton, remove the unwanted parts of the skeleton and then find a new pattern which matches the modified skeleton. (If ICU doesn't provide an API to get the skeleton, the skeleton needs to be retrieved from the pattern. There are some cases where computing a skeleton from a pattern gives slightly worse results than directly using a skeleton, but I don't know if this matters here.)
The spec could maybe then do something like:
- Determine the base format using
DateTimeStyleFormat(dateStyle, timeStyle, styles). - Extract the relevant parts from the returned DateTime Format Record into a new options Record.
- Call
GetDateTimeFormatwith that new options Record.
I don't know how good the results will be when using this approach.
There was a problem hiding this comment.
Thanks. For the moment, I've updated this PR with a prose description of what should happen. I'll see if that can be improved.
There was a problem hiding this comment.
@anba I've updated the PR again with a better description of this. It is similar to what you suggested, except that I don't think we really need to call GetDateTimeFormat since we know whether relevant options are present or not, and we don't need defaults. Instead we can just pass the options directly to the format matcher. Please let me know if this works for you.
ptomato
force-pushed
the
editorial
branch
2 times, most recently
from
3410769 to
2bd89f8
Compare
|
The
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2995 +/- ##
==========================================
- Coverage 95.88% 95.88% -0.01%
==========================================
Files 21 21
Lines 9605 9674 +69
Branches 1793 1836 +43
==========================================
+ Hits 9210 9276 +66
- Misses 342 345 +3
Partials 53 53 ☔ View full report in Codecov by Sentry. |
ptomato
force-pushed
the
editorial
branch
3 times, most recently
from
78f4034 to
e5dccd8
Compare
…sets This did not actually lead to any buggy behaviour, as far as I could tell. But it should match the spec text, which parses UTCOffset[~SubMinutePrecision] here.
This is a follow-up request from Anba, to the normative change in #2925. It moves syntactic validation of month codes and offset strings into PrepareCalendarFields. This allows implementations to store month codes and offset strings as integers in their equivalents of Calendar Fields Records, instead of allocated strings. Closes: #2962
It is valid to format e.g. a PlainDate with only { era }, or a PlainTime
with only { fractionalSecondDigits }, as those things are part of the data
model.
See: #2795
Somewhere along the line the spec text got out of sync with the consensus, probably in one of the rebases on newer ECMA-402. When attempting to format a Temporal object, if the DateTimeFormat has options that do not overlap with the data model of the Temporal object, toLocaleString() and format() are supposed to throw a TypeError. Previously, this worked correctly with dateStyle and timeStyle, but not with other options. We would treat the options as if the DateTimeFormat had been created with only default options, which was incorrect. Closes: #2795
… types timeStyle: 'long' and timeStyle: 'full' include the time zone name for legacy Date formatting. However, PlainTime and PlainDateTime don't include a time zone in their data model, so they shouldn't format a time zone name when using timeStyle. Similarly, PlainYearMonth and PlainMonthDay shouldn't format a day or year respectively when using dateStyle. This was already the consensus but somewhere along the line the spec text got out of sync, probably in a rebase on latest ECMA-402. In the polyfill, we already handled dateStyle for PlainYearMonth and PlainMonthDay. Try to hack around this the same way for timeStyle. Closes: #2795
Fixes #2962 and #2795. Includes a start on #2948 as well.
Test262 tests are in tc39/test262#4248.
Best reviewed commit by commit. I'd especially appreciate extra eyes on the spec text in "Throw TypeError if no overlap between options and Temporal object" and "Avoid formatting irrelevant parts of dateStyle/timeStyle for Temporal types", as I'm less confident that I know what I'm doing in CreateDateTimeFormat.