Normative: Add hourCycle to opt before passing to FormatMatcher #571

longlho · 2021-04-30T03:15:36Z

fixes #511

There's another alternative that I considered which is adding hourCycle to Table 4, but that would be a rather big change as hourCycle isn't technically a date/time formatting parts, just preferences.
Since it's not in Table 4, BasicFormatMatcher basically doesn't do anything with it, but BestFitFormatMatcher technically can since it's impl-dependent

ryzokuken

Are you sure this is editorial?

longlho · 2021-04-30T20:44:13Z

So I'm not sure tbh. If an implementation of BestFitFormatMatcher does use it it'll be normative. However BasicFormatMatcher doesn't use it so nothing really changes.

ryzokuken · 2021-04-30T20:56:07Z

Well, I'm pretty sure there are good reasons for a best fit matcher to take that into account, atleast if there is data available. Wouldn't the behavior of formatjs change because of this too?

I guess the actual output is not something folks can reliably predict which is why it's not a breaking change per-se, but I do feel that this would be normative. That said, it seems to be a good normative change, so I think we should land this nonetheless.

longlho · 2021-04-30T21:30:22Z

yup so formatjs/formatjs@1915595 was the driver behind this change from formatjs side :) I'm ok w/ this being normative since I am not 100% sure the boundary between the 2.
I've updated the PR title to Normative.

Setting hour12 instead of hourCycle is another option as well so that's something we can discuss.

Ref impl from formatjs: https://github.com/formatjs/formatjs/blob/main/packages/ecma402-abstract/DateTimeFormat/BestFitFormatMatcher.ts#L32-L36

ryzokuken

LGTM as a normative change.

spec/datetimeformat.html

ryzokuken

Didn't realize before Richard's comment that there were two _opt_. What was even more confusing was that the first one already had hour cycle information...

This looks perfect now! Thanks both of you!

anba · 2021-05-28T09:01:57Z

I'm not sure this change is correct to do when comparing it with how the specification currently handles hour cycles:

When dateTimeFormat.[[HourCycle]] is copied into formatOptions per the currently proposed patch, dateTimeFormat.[[HourCycle]] is null when either no hour cycle option is present (no "hourCycle" option nor hc Unicode extension) or if the "hour12" option is set. But I guess the intention is to also to select different patterns depending on the "hour12" option, right?
And, as discused in the meeting, we also already have [[pattern]] and [[pattern12]] to handle hour cycles.

It almost seems better to me to somehow rework [[pattern12]] and handle different field widths due to hour cycles completely different, instead of further complicating hour cycle handling. Well, actually we'd even need to rework the complete option to pattern computation to handle cases like https://bugzilla.mozilla.org/show_bug.cgi?id=1298794.

Btw, #454 is the same issue and has an example to show why implementations aren't currently spec-compliant.

ryzokuken · 2021-05-28T14:12:55Z

2021-05-25: This PR got TC39-TG1 consensus.

ryzokuken · 2021-06-03T17:27:08Z

2021-06-03: This PR got TC39-TG2 consensus.

sffc · 2021-06-03T22:04:08Z

The discussion from 2021-06-03 was to move forward with this PR but follow up with Anba's suggestions in a follow-up issue. Could that follow-up issue be filed?

ryzokuken · 2021-06-07T09:17:19Z

@gibson042 could you take another look at this?

@anba would you like to file the issues for your comments above?

anba · 2021-06-08T08:06:50Z

We should at least handle the hour12 issue as part of this PR.

Here's the example from #454:

js> new Date("2020-01-01T01:02:03").toLocaleString("en-u-hc-h12")
"1/1/2020, 1:02:03 AM"
js> new Date("2020-01-01T01:02:03").toLocaleString("en-u-hc-h23") 
"1/1/2020, 01:02:03"

This result isn't currently allowed in the spec, because the hour part is formatted differently ("numeric" v.s. "2-digit"). This PR attempts to solve this issue.

Now let's compare the output for hour12:

js> new Date("2020-01-01T01:02:03").toLocaleString("en", {hour12: true})
"1/1/2020, 1:02:03 AM"
js> new Date("2020-01-01T01:02:03").toLocaleString("en", {hour12: false})
"1/1/2020, 01:02:03"

This output is also currently not allowed, because the hour part is formatted differently, too. The PR in its current form doesn't cover this case, so that output will still be a spec violation.

It's easy to update the PR to handle the hour12 part, therefore I think that update should happen before merging the pull request.

ryzokuken · 2021-06-08T12:20:58Z

Sounds good to me, thanks @anba for the descriptive example. @longlho what do you think about including hour12 too?

ryzokuken · 2021-06-08T12:30:50Z

I am curious: does hour12 support in this API predate the hc/hourCycle support? It seems like the latter is more descriptive and better standardized, maybe we should be deprecating hour12 instead? (I still don't oppose including it in the formatOptions, that seems like a good idea nonetheless).

anba · 2021-06-08T13:17:38Z

Just to avoid confusion: hour12 shouldn't be passed directly to the format-matchers, but instead the default hour-cycle computation in steps 40.a-e should be moved so it happens earlier. Something like:

Let formatOptions be a new Record.

Let dataLocaleData be localeData.[[<dataLocale>]].

Let hcDefault be dataLocaleData.[[hourCycle]].

Let hc be dateTimeFormat.[[HourCycle]].

...

...

Set dateTimeFormat.[[HourCycle]] to hc.

Set formatOptions.[[hourCycle]] to dateTimeFormat.[[HourCycle]].

longlho · 2021-06-13T02:17:00Z

@anba mind taking another look? thanks!

longlho · 2021-08-17T00:44:26Z

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

spec/datetimeformat.html

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

spec/datetimeformat.html

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

longlho · 2021-09-04T17:04:40Z

gentle ping @anba :)

longlho · 2021-09-29T04:05:21Z

ping @ryzokuken

ryzokuken

@anba would you like to take a final look? I'd like to see this one merged.

ryzokuken · 2022-01-06T10:09:00Z

Looks like @anba cannot review this right now. Since this has both TG1 and TG2 consensus and signoffs from editors, let's merge this. If something still remains, it can be addressed in a follow-up.

Thanks @longlho both for the PR and your patience.

General changes: - Synchronise with latest ECMA-402 draft, including changes from tc39/ecma402#901. - Change `CreateDateTimeFormat` to always store the hour-cycle in `dateTimeFormat.[[HourCycle]]`, so later formatting operations can select the correct pattern string. Update `Intl.DateTimeFormat.prototype.resolvedOptions` to only output "hourCycle" and "hour12" when `[[DateTimeFormat]]` actually has an `[[hour]]` field. This is needed to match the existing behaviour. - Change `Value Format Records` to prefer upper-case fields and replace `[[pattern]]` and `[[rangePatterns]]` with `[[Format]]` which holds a `DateTime Format Record`. - Correctly specify `CreateTemporalDateTime` as infallible in `HandleDateTimeTemporal{Date,MonthDay,Time}`. `CreateTemporalDateTime` in `HandleDateTimeTemporalYearMonth` is fallible, because the ISO reference day can make it an out-of-range date-time value. - Remove unreachable case for a `null` format record in `HandleDateTimeTemporal{DateTime,Instant}`. - Correct value conversion in `HandleDateTimeOthers`, because `ℤ` can't be applied on a Number value. - Update `Temporal.ZonedDateTime.prototype.toLocaleString` to accept offset time zones, now that upstream ECMA-402 supports offset time zones. - Add standalone "month" as a required format to the `[[formats]]` list. This change is needed to correctly process `Temporal.PlainYearMonth` and `Temporal.PlainMonthDay`, because right now only "year+month" and "month+day" are required formats. When the user requests a standalone "month", `BasicFormatMatcher` will compute the same penalty for "year+month" and "month+day", so both formats have the same preference. This will then result in either incorrectly using "year+month" with `Temporal.PlainMonthDay` or using "month+day" with `Temporal.PlainYearMonth`. This incorrect case can't apply when standalone "month" is a required format. - It's not possible to request that all options are supported in a standalone context, because for example "year" with "en-u-ca-islamic" will currently also add "era" to the output string in implementations. Notes: - `FormatDateTimePattern` is now an infallible operation, but adding "ins" / "del" marker tags doesn't seem to work in structured types definitions. --- Updated date-time format selection: Case 1: `dateStyle` or `timeStyle` is present. When one of the style options is present, then the previous condition > If bestFormat does not have any fields that are in Supported Fields, [...] is equivalent to testing which of `dateStyle` or `timeStyle` is `undefined`, because for example when `dateStyle` isn't `undefined`, then "year", "month", and "day" are guaranteed to be present. That also means only a single call to `DateTimeStyleFormat` is needed to compute the correct format for all types. Case 2: `dateStyle` or `timeStyle` are both absent. Add the new abstract operation `GetDateTimeFormat` which implements the same formatting options processing which is currently inlined in `CreateDateTimeFormat`. The logic is now as follows: - If at least one option from `requiredOptions` is present, then use the user inputs. - Otherwise use the default options from the `defaultOptions` list. - `requiredOptions` and `defaultOptions` have different entries based on the `required` and `defaults` arguments. - The `inherit` parameter selects which user options in addition to the members of `requiredOptions` are copied over: - When `inherit=all`, then all user inputs are copied over, even if they are unexpected in the context given by `required` and `defaults`. This is needed for backwards compatibility with the current ECMA-402 spec. For example `new Date().toLocaleTimeString("en", {era: "long"})` currently returns `"Anno Domini, 7:46:53 AM"` in all engines, which means the "era" option is used even though the formatting context is a local-time string. - When `inherit=relevant`, only relevant additional options are copied over. These are options which are relevant, but can't be elements of `requiredOptions`: - "era" can't occur in a standalone context, there it's not a member of the `requiredOptions` list. - "hourCycle" is needed for the pattern selection, see tc39/ecma402#571. - The `inherit=relevant` case only applies to `Temporal.PlainX` types to ignore unsupported options. For example `new Intl.DateTimeFormat("en", {day: "numeric"}).format(new Temporal.PlainTime())` should return the string `"12:00:00 AM"` and don't display any "day" parts. - `Temporal.Instant` and `Temporal.ZonedDateTime` support all date-time options, so no additional filtering is needed for them. - In addition to "era", the "timeZoneName" option also can't occur in a standalone context, therefore it's also not an element of the `requiredOptions` list. The default options processing in GetDateTimeFormat matches the previous Temporal draft, but it's not clear at this point if this implements previous concensus, because it means the unsupported format case can only apply when `dateStyle` or `timeStyle` are used. IOW `new Intl.DateTimeFormat("en", {day: "numeric"})` works with all `Temporal` types, including `Temporal.PlainTime`, whereas `new Intl.DateTimeFormat("en", {dateStyle: "short"})` throws when used with `Temporal.PlainTime`.

longlho force-pushed the 511 branch 2 times, most recently from d812000 to 92053a2 Compare April 30, 2021 03:21

ryzokuken reviewed Apr 30, 2021

View reviewed changes

longlho changed the title ~~Editorial: Add hourCycle to opt before passing to FormatMatcher~~ Normative: Add hourCycle to opt before passing to FormatMatcher Apr 30, 2021

ryzokuken approved these changes May 3, 2021

View reviewed changes

gibson042 reviewed May 6, 2021

View reviewed changes

spec/datetimeformat.html Outdated Show resolved Hide resolved

Normative: Add hourCycle to opt before passing to FormatMatcher

c922db7

longlho force-pushed the 511 branch from 92053a2 to c922db7 Compare May 10, 2021 02:51

ryzokuken approved these changes May 10, 2021

View reviewed changes

longlho requested a review from gibson042 May 23, 2021 02:45

address comments

7bb8bc4

small refactor

f66af43

ryzokuken added the normative label Jul 22, 2021

gibson042 added a commit to gibson042/ecma402 that referenced this pull request Aug 17, 2021

Editorial: Align with tc39#571

d40d268

gibson042 reviewed Aug 17, 2021

View reviewed changes

spec/datetimeformat.html Outdated Show resolved Hide resolved

spec/datetimeformat.html Outdated Show resolved Hide resolved

Update spec/datetimeformat.html

c78beca

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

longlho added 2 commits August 17, 2021 20:50

move hourCycle resolution down to closer to matcher initialization

76694dc

move more stuff down

a4c43e3

gibson042 approved these changes Aug 18, 2021

View reviewed changes

spec/datetimeformat.html Outdated Show resolved Hide resolved

spec/datetimeformat.html Show resolved Hide resolved

Update spec/datetimeformat.html

2b9a51e

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

ryzokuken approved these changes Dec 15, 2021

View reviewed changes

ryzokuken merged commit 20e5c26 into tc39:master Jan 6, 2022

longlho deleted the 511 branch January 7, 2022 01:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Add hourCycle to opt before passing to FormatMatcher #571

Normative: Add hourCycle to opt before passing to FormatMatcher #571

longlho commented Apr 30, 2021

ryzokuken left a comment

longlho commented Apr 30, 2021 •

edited

Loading

ryzokuken commented Apr 30, 2021 •

edited

Loading

longlho commented Apr 30, 2021 •

edited

Loading

ryzokuken left a comment

ryzokuken left a comment

anba commented May 28, 2021

ryzokuken commented May 28, 2021

ryzokuken commented Jun 3, 2021

sffc commented Jun 3, 2021 •

edited

Loading

ryzokuken commented Jun 7, 2021

anba commented Jun 8, 2021

ryzokuken commented Jun 8, 2021

ryzokuken commented Jun 8, 2021

anba commented Jun 8, 2021

longlho commented Jun 13, 2021

longlho commented Aug 17, 2021 •

edited

Loading

longlho commented Sep 4, 2021

longlho commented Sep 29, 2021

ryzokuken left a comment

ryzokuken commented Jan 6, 2022

Normative: Add hourCycle to opt before passing to FormatMatcher #571

Normative: Add hourCycle to opt before passing to FormatMatcher #571

Conversation

longlho commented Apr 30, 2021

ryzokuken left a comment

Choose a reason for hiding this comment

longlho commented Apr 30, 2021 • edited Loading

ryzokuken commented Apr 30, 2021 • edited Loading

longlho commented Apr 30, 2021 • edited Loading

ryzokuken left a comment

Choose a reason for hiding this comment

ryzokuken left a comment

Choose a reason for hiding this comment

anba commented May 28, 2021

ryzokuken commented May 28, 2021

ryzokuken commented Jun 3, 2021

sffc commented Jun 3, 2021 • edited Loading

ryzokuken commented Jun 7, 2021

anba commented Jun 8, 2021

ryzokuken commented Jun 8, 2021

ryzokuken commented Jun 8, 2021

anba commented Jun 8, 2021

longlho commented Jun 13, 2021

longlho commented Aug 17, 2021 • edited Loading

longlho commented Sep 4, 2021

longlho commented Sep 29, 2021

ryzokuken left a comment

Choose a reason for hiding this comment

ryzokuken commented Jan 6, 2022

longlho commented Apr 30, 2021 •

edited

Loading

ryzokuken commented Apr 30, 2021 •

edited

Loading

longlho commented Apr 30, 2021 •

edited

Loading

sffc commented Jun 3, 2021 •

edited

Loading

longlho commented Aug 17, 2021 •

edited

Loading