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: Allow annotations after YYYY-MM and MM-DD #2398

Closed
wants to merge 1 commit into from

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Sep 1, 2022

In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations (time zone,
calendar, and unknown after #2397 lands) after the short YYYY-MM
PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This PR makes the following choices:

  • UTC offset is allowed after MM-DD
  • UTC offset is disallowed after YYYY-MM (even if it is positive, or
    contains a minute component, which would not be ambiguous)

Other choices are certainly possible.

Closes: #2379

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #2398 (03bff3f) into main (b4d9abc) will increase coverage by 0.06%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #2398      +/-   ##
==========================================
+ Coverage   95.03%   95.10%   +0.06%     
==========================================
  Files          20       20              
  Lines       10818    10829      +11     
  Branches     1925     1928       +3     
==========================================
+ Hits        10281    10299      +18     
+ Misses        503      495       -8     
- Partials       34       35       +1     
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.19% <57.14%> (-0.13%) ⬇️
polyfill/lib/regex.mjs 100.00% <100.00%> (ø)
polyfill/runtest262.mjs 87.95% <0.00%> (+4.74%) ⬆️

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

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 1, 2022

Note that I've used the new "Annotated" naming from #2397 for the new grammar nonterminals, but I haven't renamed the old ones yet. This should probably be combined into one PR with #2394, #2395, and #2397, but I'm keeping it separate for now for ease of review.

When combined with #2395, the check for "iso8601" in ParseISODateTime would need to check the ASCII-lowercase of the parsed calendar name. When combined with #2397, multiple annotations would need to be supported.

@ptomato ptomato force-pushed the 2379-annotation-yearmonth-monthday branch from bff8261 to 73ed0d0 Compare September 1, 2022 00:15
In keeping with the IXDTF format which extends the grammar of RFC 3339
with any number of annotations, we should allow annotations (time zone,
calendar, and unknown after #2397 lands) after the short YYYY-MM
PlainYearMonth and MM-DD PlainMonthDay forms.

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD. This PR makes the following choices:

- UTC offset is allowed after MM-DD
- UTC offset is disallowed after YYYY-MM (even if it is positive, or
  contains a minute component, which would not be ambiguous)

Other choices are certainly possible.

Closes: #2379
@ptomato ptomato force-pushed the 2379-annotation-yearmonth-monthday branch from 73ed0d0 to 03bff3f Compare September 1, 2022 00:17
@sffc
Copy link
Collaborator

sffc commented Sep 1, 2022

If we were to allow UTC offsets ±UU[:UU] alongside the time zone
annotation, that would be ambiguous in one case: YYYY-MM-UU would be
ambiguous with YYYY-MM-DD

Is this a bug in IXDTF? The grammar should not be ambiguous.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 1, 2022

Is this a bug in IXDTF? The grammar should not be ambiguous.

IXDTF seems fine:

date-time-ext = date-time suffix         // IXDTF
date-time     = full-date "T" full-time  // RFC 3339

Anything with less than a full date / full time isn't covered in RFC 3339, only in its ISO 8601 appendix.

@sffc
Copy link
Collaborator

sffc commented Sep 1, 2022

This just smells really fishy to me. I thought the point of IXDTF was to make it such that Temporal's datetime syntax was written down somewhere other than in ECMA-262.

This is problematic for ICU4X because we're writing an IXDTF parser with the idea that it will be compliant with Temporal, but it sounds like doing so is not sufficient since Temporal has further extensions on top of IXDTF.

Basically, we have RFC 3339, ISO-8601-1, ISO-8601-2, IXDTF, and Temporal that all have slightly different rules.

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 1, 2022

I don't think that line of reasoning leads to a desirable outcome for anyone...

RFC 3339's domain, same as IXDTF's, is the full-date full-time timestamp format, corresponding with Instant. We've always known that, and have in the past assumed that whatever annotation we standardize there is fine to carry over, with the necessary changes, to the Plain formats, because ISO 8601 allows omitting components.

If we don't want to have annotations after YYYY-MM and MM-DD that's debatable (although we should have that discussion over at #2379, and preferably before the plenary.)

@ptomato
Copy link
Collaborator Author

ptomato commented Sep 5, 2022

Draft until presented at plenary (also depending on the outcome of the discussion at #2379)

@ptomato ptomato marked this pull request as draft September 5, 2022 17:53
@ptomato
Copy link
Collaborator Author

ptomato commented Sep 15, 2022

This needs updating to the new plan discussed in #2379. I'll close it for now.

@ptomato ptomato closed this Sep 15, 2022
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 24, 2022

Updated to the new plan from #2379 (comment).

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 24, 2022

Never mind, GitHub won't let me reopen the PR. A new PR is at #2428.

ptomato added a commit to ptomato/test262 that referenced this pull request Oct 24, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit to ptomato/test262 that referenced this pull request Nov 29, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit to ptomato/test262 that referenced this pull request Nov 30, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
ptomato added a commit to tc39/test262 that referenced this pull request Nov 30, 2022
As per the discussion in
tc39/proposal-temporal#2379 (comment)
and the PR tc39/proposal-temporal#2398, which is
to be presented for consensus to TC39 in the upcoming plenary meeting, UTC
offsets and the Z designator should be disallowed after any date-only
strings (YYYY-MM-DD, YYYY-MM, and MM-DD). They should only be allowed to
follow a time component. Z remains disallowed in any string being parsed
into a Plain type.

Annotations become allowed after any ISO string, even YYYY-MM and MM-DD
where they were previously disallowed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

|DateSpecYearMonth| and |DateSpecMonthDay| don't allow a calendar suffix
2 participants