Editorial: Various editorial updates #827
Closed
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
…type.resolvedOptions
…] 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.
Updated to match <tc39/ecma262#2874>.
…ializeRelativeTimeFormat
Remove the separate initialisation operations `InitializeCollator`,
`InitializePluralRules`, and `InitializeRelativeTimeFormat` and instead
inline the algorithm steps into their corresponding constructor
functions.
The separate initialisation operations are a left-over from the first
ECMA-402 editions, which supported initialising arbitrary objects as
Intl objects. This capability has been removed at some point, so it's
now possible to also remove the separate initialisation operations.
`Intl.PluralRules` and `Intl.RelativeTimeFormat` also used separate
initialisation operations, but that's just because they've copied the
structure used for `Intl.{Collator,NumberFormat,DateTimeFormat}`.
Move the call to `ResolveLocale` to match the other Intl constructors.
`FindBoundary` can only return a non-finite result when called from `%SegmentIteratorPrototype%.next()`. When moving the `startIndex ≥ len` check into `%SegmentIteratorPrototype%.next()`, `FindBoundary` can be changed to return only a non-negative integer. Also removes `startIndex ≥ 0` assertions from `FindBoundary` and `CreateSegmentDataObject`, because `startIndex` is guaranteed to be non-negative through the structured header declaration.
`result` is not used when the `FormatApproximately` code path is taken.
Follow the main spec and simply use "Append `thing` to `list`".
This matches the main spec.
…mpare's "length" property Also update to the standard description: > The "length" property of this function is ...
Two issues: 1. `"length" property of fv` isn't the correct term to determine a string length. 2. And it doesn't correctly handle digits which contain surrogate pairs. Use `StringToCodePoints` to correctly determine the number of code points and extract the last two code points. And then call `CodePointsToString` to compute the resulting string. `CodePointsToString` is declared as receiving a `sequence of Unicode code points`, not a `List of Unicode code points`, but it seems like the `sequence` and `List` types are interchangeable, at least based on other uses in the ECMA-262 spec.
ben-allen
reviewed
Sep 18, 2023
There was a problem hiding this comment.
whew! there's places where it seems like you missed changing all of the instances of the thing you were changing (i.e. an "append" that didn't get changed to "string-concatenation") but this is as far as I can tell as stellar as the rest of this.
I'm very tempted to put in a PR on 262 regularizing their spec as well, since there's oddities of the type you fixed still present there. or at least I will be tempted, next time I have copious free time.
| @@ -340,7 +340,7 @@ <h1> | |||
| </dl> | |||
| <emu-alg> | |||
| 1. Let _parts_ be CreatePartsFromList(_listFormat_, _list_). | |||
| 1. Let _result_ be an empty String. | |||
| 1. Let _result_ be the empty String. | |||
There was a problem hiding this comment.
nice! there's a couple of these over in 262 as well
| 1. Let _localeData_ be %PluralRules%.[[LocaleData]]. | ||
| 1. Let _r_ be ResolveLocale(%PluralRules%.[[AvailableLocales]], _requestedLocales_, _opt_, %PluralRules%.[[RelevantExtensionKeys]], _localeData_). | ||
| 1. Set _pluralRules_.[[Locale]] to _r_.[[locale]]. | ||
| 1. Let _t_ be ? GetOption(_options_, *"type"*, ~string~, « *"cardinal"*, *"ordinal"* », *"cardinal"*). |
There was a problem hiding this comment.
this had been on my to-do list for months! thank you!
| @@ -442,14 +491,14 @@ <h1>PartitionPattern ( _pattern_ )</h1> | |||
| 1. Assert: _endIndex_ is greater than _beginIndex_. | |||
| 1. If _beginIndex_ is greater than _nextIndex_, then | |||
| 1. Let _literal_ be a substring of _pattern_ from position _nextIndex_, inclusive, to position _beginIndex_, exclusive. | |||
There was a problem hiding this comment.
appears to be an unnecessary inclusive/exclusive that got through
ben-allen
approved these changes
Sep 18, 2023
|
Any objections to merging this one? rebasing is going to be more and more of a pain as time goes on. @sffc @bakkot @FrankYFTang @ryzokuken |
|
Yes, if the editors approve, please merge; we shouldn't let PRs stay open this long |
This was referenced May 24, 2024
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.
Various editorial updates, including changes to align with ECMA-262 conventions and some editorial bug fixes.
The PR applies on top of #826.