Editorial: Fix various rebase errors #901
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anba
force-pushed
the
rebase-errors
branch
3 times, most recently
from
81a16b8 to
6ab212b
Compare
anba
added a commit
to anba/proposal-temporal
that referenced
this pull request
Jun 24, 2024
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`.
|
(tc39/proposal-temporal#2904 depends on these change. This is mentioned in the commit messages for the other PR, but not in the description, so explicitly mention it here so that GH can create cross-reference links.) |
ptomato
pushed a commit
to tc39/proposal-temporal
that referenced
this pull request
Jul 1, 2024
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`.
ben-allen
approved these changes
Jul 10, 2024
There was a problem hiding this comment.
Thanks for this! Barring objections, I'll merge it midday tomorrow @gibson042
ben-allen
reviewed
Jul 12, 2024
| @@ -1403,31 +1412,31 @@ <h1> | |||
| 1. Perform ! CreateDataPropertyOrThrow(_nf2Options_, *"minimumIntegerDigits"*, *2*<sub>𝔽</sub>). | |||
| 1. Perform ! CreateDataPropertyOrThrow(_nf2Options_, *"useGrouping"*, *false*). | |||
| 1. Let _nf2_ be ! Construct(%Intl.NumberFormat%, « _locale_, _nf2Options_ »). | |||
| 1. Let _fractionalSecondDigits_ be _dateTimeFormat_.[[FractionalSecondDigits]]. | |||
| 1. If _fractionalSecondDigits_ is *undefined*, set _fractionalSecondDigits_ to 1. | |||
There was a problem hiding this comment.
I was looking back over this one while reviewing #903 and I realized I had some questions about the removal of this line. The message for the commit where it was added (731d597) observes that _fractionalSecondDigits_ being *undefined* here is a theoretical case / doesn't affect implementations; is it worthwhile to either keep this line in or else leave a note about it?
There was a problem hiding this comment.
The issue described in 731d597 no longer applies, because the number of fractional second digits are now directly read from the format record instead of the Intl.DateTimeFormat object.
Before 731d597 it was possible that PartitionDateTimeRangePattern calls FormatDateTimePattern with a patternPart whose [[Type]] is "fractionalSecondDigits". Step 15.c.iii then performs:
Let fv be FormatNumeric(nf3, v).
But nf3 is only defined when dateTimeFormat.[[FractionalSecondDigits]] is not undefiend, cf. steps 11-12.
This case can no longer apply in the current FormatDateTimePattern operation, because if the pattern string pattern contains the substring "{fractionalSecondDigits}", then format has a field [[fractionalSecondDigits]], cf. steps 9 and 13.c.i. And the requirements in Table 5:
Optional field. Present if [[pattern]] contains the substring "{fractionalSecondDigits}".
And Table 7:
Optional field. Present if a Pattern String in [[PatternParts]] contains the substring "{fractionalSecondDigits}".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a couple of rebase errors. Also reverts attempts to address review feedback ("pattern" instead of "format" for some records), because the changes were largely unfinished and actually using "pattern" would result in equally confusing code like
pattern.[[pattern]]which doesn't seem like an improvement when compared toformat.[[pattern]]. For example see FormatDateTimePattern, which hasformatandpatternarguments.Fixes #899.