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

Normative: Reject an ambiguous time string even with a calendar #2287

Merged
merged 4 commits into from
Aug 16, 2022

Conversation

gibson042
Copy link
Collaborator

Fixes #2285

@gibson042 gibson042 requested review from justingrant and ptomato June 10, 2022 19:07
@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #2287 (4a6f8c7) into main (5a84124) will decrease coverage by 0.00%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #2287      +/-   ##
==========================================
- Coverage   95.03%   95.02%   -0.01%     
==========================================
  Files          20       20              
  Lines       10812    10817       +5     
  Branches     1927     1929       +2     
==========================================
+ Hits        10275    10279       +4     
- Misses        503      504       +1     
  Partials       34       34              
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.30% <90.00%> (-0.02%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Agreed with the reasoning in #2285. Let's present this at the next TC39, marking as draft until then.

@ptomato ptomato marked this pull request as draft June 24, 2022 10:58
@ptomato ptomato added the normative Would be a normative change to the proposal label Jul 4, 2022
ptomato added a commit to ptomato/test262 that referenced this pull request Aug 16, 2022
This implements the normative change in
tc39/proposal-temporal#2287 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure that PlainTime strings which require a T
designator for disambiguation, are not disambiguated by adding a calendar
annotation.
@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

This gained consensus at the July 2022 TC39 plenary. Tests are now available in tc39/test262#3641. I had some last minute doubts about whether this one was actually correct, but I'm convinced by the discussion in #2285.

@ptomato ptomato marked this pull request as ready for review August 16, 2022 23:27
@ptomato ptomato force-pushed the gh-2285-ambiguous-calendartime branch from 7bf3b3f to 4a6f8c7 Compare August 16, 2022 23:28
@ptomato ptomato merged commit 08b4a13 into tc39:main Aug 16, 2022
Ms2ger pushed a commit to ptomato/test262 that referenced this pull request Aug 31, 2022
This implements the normative change in
tc39/proposal-temporal#2287 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure that PlainTime strings which require a T
designator for disambiguation, are not disambiguated by adding a calendar
annotation.
ptomato added a commit to ptomato/test262 that referenced this pull request Aug 31, 2022
This implements the normative change in
tc39/proposal-temporal#2287 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure that PlainTime strings which require a T
designator for disambiguation, are not disambiguated by adding a calendar
annotation.
ptomato added a commit to tc39/test262 that referenced this pull request Aug 31, 2022
This implements the normative change in
tc39/proposal-temporal#2287 which reached
consensus at the July 2022 TC39 meeting.

It adds tests that ensure that PlainTime strings which require a T
designator for disambiguation, are not disambiguated by adding a calendar
annotation.
pull bot pushed a commit to jamlee-t/v8 that referenced this pull request Sep 10, 2022
Add TimeHourMinuteBasicFormatNotAmbiguousWithMonthDay
TimeZoneNumericUTCOffsetNotAmbiguousWithDayOfMonth
TimeZoneNumericUTCOffsetNotAmbiguousWithMonth
TimeZoneIdentifier, UnpaddedHour, TimeZoneIANALegacyName productions.

Sync the spec of TemporalInstantString, TemporalTimeString
TimeZone, TimeZoneBracketedAnnotation, TemporalTimeZoneString,
ToTemporalTimeZone, TimeZoneIANAName productions.

Fix bug in ScanCalendarDateTimeTimeRequired, ToTemporalTimeZone

Change name from Handle<String> to Handle<Object> to hold undefined

Update parser tests accordingly.

Spec Text:
https://tc39.es/proposal-temporal/#sec-temporal-iso8601grammar
https://tc39.es/proposal-temporal/#sec-temporal-totemporaltimezone


Related PR changes:
tc39/proposal-temporal#2284
tc39/proposal-temporal#2287
tc39/proposal-temporal#2345


Bug: v8:11544
Change-Id: I6f1a5e5dedba461db9f36abe76fa97119c1f8c2c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3822342
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83123}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

|TemporalTimeString| accepts ambiguous input with a calendar
3 participants