Normative: Support 'T' prefix in time-only strings and require it in cases of ambiguity #1952

ptomato · 2021-12-03T02:46:53Z

Includes #1950 to avoid rebase conflicts. I'll make sure that one is merged before this one.

Closes: #1765

codecov · 2021-12-03T02:47:54Z

Codecov Report

Merging #1952 (bc4b64c) into main (7e58ba3) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1952      +/-   ##
==========================================
+ Coverage   94.96%   94.97%   +0.01%     
==========================================
  Files          19       19              
  Lines       10967    10989      +22     
  Branches     1743     1751       +8     
==========================================
+ Hits        10415    10437      +22     
  Misses        535      535              
  Partials       17       17

Flag	Coverage Δ
test262	`79.26% <71.42%> (-0.04%)`	⬇️
tests	`89.88% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
polyfill/lib/ecmascript.mjs	`95.27% <100.00%> (+0.02%)`	⬆️
polyfill/lib/regex.mjs	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e58ba3...bc4b64c. Read the comment docs.

ptomato · 2021-12-03T02:53:01Z

Draft until presented at TC39

spec/abstractops.html

ptomato · 2021-12-17T18:39:07Z

This achieved consensus at the December 2021 TC39 meeting.

This prefix should always have been allowed according to ISO 8601. We had neglected to implement it. See: #1765

These now need to be parsed as follows: Temporal.PlainDateTime.from(string).toPlainTime() See #1765

ISO 8601 requires the 'T' prefix in cases where the time-only representation is ambiguous. Closes: #1765

polyfill/lib/ecmascript.mjs

Slightly refector the implementation of tc39#1952 to enable a clearer error message when the user tries to parse a date-only string with `PlainTime.from`. This is the same pattern that's used for other polyfill parsing: `ParseISODateTime` is used for generic, regex-only parsing while type-specific validation happens in the calling method, e.g. `ParseTemporalTimeString` or `ParseTemporalZonedDateTimeString`. Also add tests to verify that date strings are not allowed, and that space is not accepted as a time designator prefix even though space is accepted as a date/time separator.

Slightly refector the implementation of #1952 to enable a clearer error message when the user tries to parse a date-only string with `PlainTime.from`. This is the same pattern that's used for other polyfill parsing: `ParseISODateTime` is used for generic, regex-only parsing while type-specific validation happens in the calling method, e.g. `ParseTemporalTimeString` or `ParseTemporalZonedDateTimeString`. Also add tests to verify that date strings are not allowed, and that space is not accepted as a time designator prefix even though space is accepted as a date/time separator.

Port of two overlapping PRs (the latter is a revision of the former): * tc39/proposal-temporal#1952 * tc39/proposal-temporal#1986

tc39/proposal-temporal#1952 added support for time designator prefixes in PlainTime strings. This adds three tests to all entry points that convert an ISO string to a PlainTime: - no-implicit-midnight: ISO strings with only a date and no time are no longer accepted. Previously they were implicitly interpreted as 00:00. - with-time-designator: Tests that various forms of string with time designator are correctly parsed. - time-designator-required-for-disambiguation: Tests various cases where a string without a time designator is ambiguous and therefore the time designator is required, as well as various cases that implementations might assume are ambiguous but in fact are not. This was a normative change that achieved consensus at the December 2021 TC39 meeting.

FrankYFTang · 2022-05-09T06:22:13Z

Why do we need to introduce "CalendarDate" which is only used in an Assert?

$ egrep "CalendarDate[^a-zA-Z]" *
abstractops.html: CalendarDate :
abstractops.html: 1. Assert: ParseText(StringToCodePoints(isoString), |CalendarDate|) is a List of errors.

Could we remove that from that spec?

ptomato · 2022-05-09T20:42:09Z

This is done in #2187.

Spec text: https://tc39.es/proposal-temporal/#sec-temporal-iso8601grammar Support 'T' prefix in time-only strings and require it in cases of ambiguity Remove TemporalDateString and TemporalRelativeToString from parser Change algorithm of ParseTemporalDateString Related spec changes: tc39/proposal-temporal#1952 tc39/proposal-temporal#2187 Bug: v8:11544 Change-Id: I7430afabb7dd78930b339b818bad7c7721decb99 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3636361 Reviewed-by: Shu-yu Guo <[email protected]> Commit-Queue: Frank Tang <[email protected]> Cr-Commit-Position: refs/heads/main@{#81792}

ptomato requested review from Ms2ger, justingrant, gibson042 and sarahghp December 3, 2021 02:46

ptomato marked this pull request as draft December 3, 2021 02:53

ptomato added the spec-text Specification text involved label Dec 3, 2021

Ms2ger approved these changes Dec 3, 2021

View reviewed changes

gibson042 reviewed Dec 7, 2021

View reviewed changes

spec/abstractops.html Show resolved Hide resolved

ptomato marked this pull request as ready for review December 17, 2021 18:39

ptomato mentioned this pull request Dec 17, 2021

Use early errors to simplify ISO 8601 grammar #1984

Closed

ptomato added 2 commits December 17, 2021 11:23

Normative: Allow 'T' prefix in time-only strings

5b924bd

This prefix should always have been allowed according to ISO 8601. We had neglected to implement it. See: #1765

Normative: Disallow date-only strings for PlainTime

7c90708

These now need to be parsed as follows: Temporal.PlainDateTime.from(string).toPlainTime() See #1765

ptomato force-pushed the 1765-time-only-repr branch from 04a13c5 to c380e4f Compare December 17, 2021 19:31

Normative: Require 'T' prefix for ambiguous time-only strings

bc4b64c

ISO 8601 requires the 'T' prefix in cases where the time-only representation is ambiguous. Closes: #1765

ptomato force-pushed the 1765-time-only-repr branch from c380e4f to bc4b64c Compare December 17, 2021 19:35

ptomato merged commit 514ede3 into main Dec 17, 2021

ptomato deleted the 1765-time-only-repr branch December 17, 2021 20:15

justingrant reviewed Dec 18, 2021

View reviewed changes

polyfill/lib/ecmascript.mjs Show resolved Hide resolved

justingrant mentioned this pull request Dec 19, 2021

Improve "missing time in string" error message #1986

Merged

justingrant mentioned this pull request Dec 20, 2021

Port of proposal-temporal PRs: 1950, 1952+1986, 1956 js-temporal/temporal-polyfill#114

Merged

gibson042 mentioned this pull request May 9, 2022

Editorial: Remove unnecessary productions #2187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Support 'T' prefix in time-only strings and require it in cases of ambiguity #1952

Normative: Support 'T' prefix in time-only strings and require it in cases of ambiguity #1952

ptomato commented Dec 3, 2021

codecov bot commented Dec 3, 2021 •

edited

Loading

ptomato commented Dec 3, 2021

ptomato commented Dec 17, 2021

FrankYFTang commented May 9, 2022

ptomato commented May 9, 2022

Normative: Support 'T' prefix in time-only strings and require it in cases of ambiguity #1952

Normative: Support 'T' prefix in time-only strings and require it in cases of ambiguity #1952

Conversation

ptomato commented Dec 3, 2021

codecov bot commented Dec 3, 2021 • edited Loading

Codecov Report

ptomato commented Dec 3, 2021

ptomato commented Dec 17, 2021

FrankYFTang commented May 9, 2022

ptomato commented May 9, 2022

codecov bot commented Dec 3, 2021 •

edited

Loading