Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Use named records for DateTimeFormat format records #826

Closed
wants to merge 18 commits into from

Conversation

anba
Copy link
Contributor

@anba anba commented Aug 11, 2023

This PR aims to restructure Intl.DateTimeFormat Constructor - Internal Slots to use named Record types instead of ad-hoc records. In the process of this clean-up, some editorial bugs were found and fixed, see the commit messages for details.

In addition to providing a more type-safe approach when compared to using ad-hoc records, these changes should also help with the Temporal integration, see https://tc39.es/proposal-temporal/#sec-temporal-intl.


The PR applies on top of #822.

Copy link
Collaborator

@ben-allen ben-allen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whew! Reviewed commits one-by-one; most of the comments are about things fixed in later commits. I've left those comments in place, but flagged them as fixed later.

Thank you again for all your work on this -- it's awe-inspiring.

spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
@anba
Copy link
Contributor Author

anba commented Oct 18, 2023

Fixed all review comments and rebased the commits to apply cleanly on tip.

…] formats

Replaces the text description of the various DateTime format records
with named Record types. This should help to document which format
records contain which fields and their corresponding values.

The new Record types currently only describe their fields. Additional
introductory information could probably added if needed.

Also fixes some editorial bugs, for example:

1. The previous format description required that if a format record has
   a `[[year]]` field, the `[[pattern]]` string must contain the
   substring `{year}`. This isn't correct when the pattern string uses
   `{yearName}` and/or `{relatedYear}`.
2. `[[rangePatterns]].[[Default]]` was spec'ed to be a list of records,
   even though it's actually a single record.
Replaces untyped Record parameters with their new named Record types.
…TimePattern

When `FormatDateTimePattern` is called from
`PartitionDateTimeRangePattern` to format a range pattern, the range
pattern could contain the substring `"fractionalSecondDigits"`, but the
DateTimeFormat's `[[FractionalSecondDigits]]` internal slot is
`undefined`. This makes operations like `10^(fractionalSecondDigits-3)`
invalid. (This also means this change has to be editorial, because it's
an internal spec bug.)

It seems difficult to describe a restriction that range patterns mustn't
include smaller calendar elements when compared to the corresponding
non-range DateTime Format Record. Therefore this commit simply defaults
`fractionalSecondDigits` to a default value when
`[[FractionalSecondDigits]]` is `undefined`.

This merely theoretical case doesn't happen in actual implementations,
so implementors can ignore this change.
Both fields should be read from `rangeFormatOptions` when formatting a
date range pattern.
Replaces `[[Pattern]]` and `[[RangePatterns]]` with access to
`[[DateTimeFormat]]`, which holds the resolved best format as a DateTime
Format Record.
And then perform the call to `PartitionPattern` in
`FormatDateTimePattern`.
Pass a format record (DateTime Format Record or a DateTime Range Pattern
Format Record) to `FormatDateTimePattern` and read the formatting options
from the format record.
When `p = "era"`, then `f = format.[[era]]`, so it doesn't make sense to
specify that the result depends on whether `format.[[era]]` is present.
`CreateDateTimeFormat` copies the resolved format options into the
Intl.DateTimeFormat object. That means we get the same results when
`resolvedOptions` reads the resolved format options from the format
record.
The previous commits removed all uses of these internal slots, so we can
now remove the internal slots, too.
This is less confusing when compared to setting and later resetting the
internal slot.
The "Range Pattern Field" column is of type `Field Name`, but the
`[[<name>]]` form can only be used when `name` is a `String` value.
Either the "Range Pattern Field" column values need to be changed to
String values, e.g. `"Era"` instead of `[[Era]]`, or the field access
needs to be rewritten. This commit uses the latter approach.
This column is never used. But also see the next commit.
The table is only used in `PartitionDateTimeRangePattern`. When
replacing that use with the new "DateTime Range Pattern Record" table,
we can remove the "Range pattern fields" table.
The structured header already ensures the parameter is a List type.
Add a simple definition for Pattern Strings and rename some parameters,
so they link to the new definition.
@anba
Copy link
Contributor Author

anba commented Dec 15, 2023

Rebased again.

Do we still wait for another review or can we land these changes?

@sffc sffc requested a review from gibson042 December 15, 2023 17:42
Copy link
Contributor

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gibson042 If this looks good please merge.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great stuff, but see my comments about surfacing "pattern" in the new names.

spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
spec/datetimeformat.html Outdated Show resolved Hide resolved
Comment on lines +363 to +384
<tr>
<td>[[rangePatterns]]</td>
<td>a DateTime Range Pattern Record</td>
<td>Pattern strings in this field are similar to [[pattern]].</td>
</tr>
<tr>
<td>[[rangePatterns12]]</td>
<td>a DateTime Range Pattern Record</td>
<td>Optional field. Present if the [[hour]] field is present. Pattern strings in this field are similar to [[pattern12]].</td>
</tr>
</table>
</emu-table>
</emu-clause>

<emu-clause id="sec-datetimeformat-range-pattern-record">
<h1>DateTime Range Pattern Records</h1>

<p>
Each <dfn id="datetimeformat-range-pattern-record">DateTime Range Pattern Record</dfn> has the fields defined in <emu-xref href="#table-datetimeformat-range-pattern-record"></emu-xref>.
</p>
<emu-table id="table-datetimeformat-range-pattern-record">
<emu-caption>DateTime Range Pattern Record</emu-caption>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a goal of replacing each [[rangePatterns]] and [[rangePatterns12]] unit-keyed Record with a List of Records with fields like { [[BiggestDivergentUnit]]: string, [[RangePattern]]: DateTimeFormat Range Pattern Record } Records (or possibly just a list of range pattern Records that include that first field, saving a level of depth), and it might be appropriate to include that change in this PR rather than introduce new bibliographic terms that will just need to be removed. Thoughts?

The net effect should be to reduce opacity of field semantics, replacing e.g.

[[rangePatterns]]: {
  [[Hour]]: {
    [[hour]]: "numeric",
    [[minute]]: "numeric",
    [[PatternParts]]: […]
  },
  [[Minute]]: {
    [[hour]]: "numeric",
    [[minute]]: "numeric",
    [[PatternParts]]: […]
  },
  [[Default]]: {
    [[year]]: "2-digit",
    [[month]]: "numeric",
    …,
    [[PatternParts]]: […]
}

with

[[rangePatterns]]: [
  { [[BiggestDivergentUnit]]: "hour",
    [[hour]]: "numeric",
    [[minute]]: "numeric",
    [[PatternParts]]: […]
  },
  { [[BiggestDivergentUnit]]: "minute",
    [[hour]]: "numeric",
    [[minute]]: "numeric",
    [[PatternParts]]: […]
  },
  { [[BiggestDivergentUnit]]: ~empty~,
    [[year]]: "2-digit",
    [[month]]: "numeric",
    …,
    [[PatternParts]]: […]
  }
]

</thead>
<tr>
<td>[[full]]</td>
<td>a DateTime Time Range Record</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also thinking about flattening these from [[DateTimeRangeFormat]].[[<dateStyle>]].[[<timeStyle>]] to [[DateTimeRangeFormat]].[[<dateAndTimeStyle>]] with e.g. Let _dateAndTimeStyle_ be the string-concatenation of _dateStyle_, *"-"*, and _timeStyle_., but haven't yet decided if that's worthwhile.

Comment on lines 418 to 423
<emu-clause id="sec-pattern-string-types">
<h1>Pattern String Types</h1>
<p>
<dfn>Pattern String</dfn> is a String value which contains zero or more substrings of the form *"{name}"*. The syntax of the abstract pattern strings is an implementation detail and is not exposed to users of ECMA-402.
</p>
</emu-clause>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

spec/locales-currencies-tz.html Outdated Show resolved Hide resolved
@ryzokuken ryzokuken added editorial Involves an editorial fix needs review labels Feb 22, 2024
Several clarifications from @gibson42

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>
@ben-allen
Copy link
Collaborator

ben-allen commented Apr 3, 2024

@anba I've rebased this onto the current main on my own fork: https://github.com/ben-allen/ecma402/tree/anba-826-working-branch. If you pull it in, I can merge.

@ben-allen
Copy link
Collaborator

@anba Rebased again! It's still at https://github.com/ben-allen/ecma402/tree/anba-826-working-branch

(also, apologies for letting this one sit so long)

@@ -60,7 +60,7 @@ <h1>
</dl>

<emu-alg>
1. Let _dateTimeFormat_ be ? OrdinaryCreateFromConstructor(_newTarget_, *"%DateTimeFormat.prototype%"*, &laquo; [[InitializedDateTimeFormat]], [[Locale]], [[Calendar]], [[NumberingSystem]], [[TimeZone]], [[Weekday]], [[Era]], [[Year]], [[Month]], [[Day]], [[DayPeriod]], [[Hour]], [[Minute]], [[Second]], [[FractionalSecondDigits]], [[TimeZoneName]], [[HourCycle]], [[DateStyle]], [[TimeStyle]], [[Pattern]], [[RangePatterns]], [[BoundFormat]] &raquo;).
1. Let _dateTimeFormat_ be ? OrdinaryCreateFromConstructor(_newTarget_, *"%DateTimeFormat.prototype%"*, &laquo; [[InitializedDateTimeFormat]], [[Locale]], [[Calendar]], [[NumberingSystem]], [[TimeZone]], [[HourCycle]], [[DateStyle]], [[TimeStyle]], [[DateTimeFormat]], [[BoundFormat]] &raquo;).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Let _dateTimeFormat_ be ? OrdinaryCreateFromConstructor(_newTarget_, *"%DateTimeFormat.prototype%"*, &laquo; [[InitializedDateTimeFormat]], [[Locale]], [[Calendar]], [[NumberingSystem]], [[TimeZone]], [[HourCycle]], [[DateStyle]], [[TimeStyle]], [[DateTimeFormat]], [[BoundFormat]] &raquo;).
1. Let _dateTimeFormat_ be ? OrdinaryCreateFromConstructor(_newTarget_, *"%DateTimeFormat.prototype%"*, &laquo; [[InitializedDateTimeFormat]], [[Locale]], [[Calendar]], [[NumberingSystem]], [[TimeZone]], [[HourCycle]], [[DateStyle]], [[TimeStyle]], [[Patterns]], [[BoundFormat]] &raquo;).

<h1>DateTimeFormat Patterns Records</h1>

<p>
Each <dfn id="datetimeformat-patterns-record" variants="DateTimeFormat Patterns Records">DateTimeFormat Patterns Record</dfn> has the fields defined in <emu-xref href="#table-datetimeformat-patterns-record"></emu-xref>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This id seems superfluous; linking to the dedicated section should be fine.

Suggested change
Each <dfn id="datetimeformat-patterns-record" variants="DateTimeFormat Patterns Records">DateTimeFormat Patterns Record</dfn> has the fields defined in <emu-xref href="#table-datetimeformat-patterns-record"></emu-xref>.
Each <dfn variants="DateTimeFormat Patterns Records">DateTimeFormat Patterns Record</dfn> has the fields defined in <emu-xref href="#table-datetimeformat-patterns-record"></emu-xref>.

<tr>
<td>[[pattern]]</td>
<td>a Pattern String</td>
<td><emu-not-ref>Contains</emu-not-ref> for each of the date and time format component fields of the record a substring starting with *"{"*, followed by the name of the field, followed by *"}"*. If the record has a [[year]] field, the string may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<td><emu-not-ref>Contains</emu-not-ref> for each of the date and time format component fields of the record a substring starting with *"{"*, followed by the name of the field, followed by *"}"*. If the record has a [[year]] field, the string may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</td>
<td><emu-not-ref>Contains</emu-not-ref> for each of the date and time format component fields of the Record a <emu-not-ref>substring</emu-not-ref> starting with *"{"*, followed by the name of the field, followed by *"}"*. If the Record has a [[year]] field, the string may contain the substrings *"{yearName}"* and *"{relatedYear}"*.</td>

<tr>
<td>[[PatternParts]]</td>
<td>a List of DateTime Range Pattern Part Records</td>
<td>Each record represents a part of the range pattern.</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<td>Each record represents a part of the range pattern.</td>
<td>Each Record represents a part of the range pattern.</td>

Comment on lines +613 to +632
<tr>
<td>[[full]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"full"* style.</td>
</tr>
<tr>
<td>[[long]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"long"* style.</td>
</tr>
<tr>
<td>[[medium]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"medium"* style.</td>
</tr>
<tr>
<td>[[short]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"short"* style.</td>
</tr>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<tr>
<td>[[full]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"full"* style.</td>
</tr>
<tr>
<td>[[long]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"long"* style.</td>
</tr>
<tr>
<td>[[medium]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"medium"* style.</td>
</tr>
<tr>
<td>[[short]]</td>
<td>a DateTime Format Record</td>
<td>Format record for the *"short"* style.</td>
</tr>
<tr>
<td>[[full]]</td>
<td>a DateTime Format Record</td>
<td>Format Record for the *"full"* style.</td>
</tr>
<tr>
<td>[[long]]</td>
<td>a DateTime Format Record</td>
<td>Format Record for the *"long"* style.</td>
</tr>
<tr>
<td>[[medium]]</td>
<td>a DateTime Format Record</td>
<td>Format Record for the *"medium"* style.</td>
</tr>
<tr>
<td>[[short]]</td>
<td>a DateTime Format Record</td>
<td>Format Record for the *"short"* style.</td>
</tr>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased again! Most of the comments above are addressed by #887 -- I think the only one that isn't is the use of id in the DateTimeFormat Patterns Record dfn

@anba @gibson042

@ben-allen
Copy link
Collaborator

ben-allen commented May 24, 2024

I've merged the rebased version of this PR and #827 via the new #893.

@ben-allen ben-allen closed this May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Involves an editorial fix needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants