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: refactor time zone offset handling (prepare for #2607) #2621

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 28, 2023

This PR refactors spec text and polyfill code for time zone offsets, splitting the handling of offsets used as time zone identifiers from offsets used for ZDT's offset property (both parsing and output), and for parsing of Instant and the RFC3339 portion of ZDT strings.

This split will prepare for the normative changes in #2607 and will make the normative PR easier to review.

Here's more details about what's in this PR:

  • Moves offset string grammar into abstractops.html, remove it from mainadditions.html
  • Splits the |UTCOffset| production into |UTCOffsetMinutePrecision| and |UTCOffsetSubMinutePrecision|.
  • Replaces the pattern of calling IsTimeZoneOffsetString, throwing if it's false, and then calling ParseTimeZoneOffsetString. Instead, offset-parsing AOs (like other Temporal parsing AOs) will throw if they're provided with an invalid string, which lets us replace two AO calls and an If statement with a single call to a parsing AO.
  • Splits ParseTimeZoneOffsetString into two new AOs for parsing ISO string offsets and offset time zone identifiers, respectively. Both return Records instead of primtives:
    • ParseUTCOffsetString returns { [[OffsetNanoseconds]: an integer, [[HasSubMinutePrecision]]: a Boolean }
    • ParseTimeZoneIdentifier returns { [[Name]]: a String or ~empty~, [[OffsetNanoseconds]]: an integer or ~empty~}
    • If the input string isn't valid, both new AOs throw a RangeError.
  • Renames IsTimeZoneOffsetString to IsOffsetTimeZoneIdentifier.
  • Updates ecmarkup to a version that supports <ins> and <del> in structured headers in order to support the renames above.
  • Aligns offset parsing in the polyfill more closely to the spec text.
  • Adds a note to ISO grammar about how we extend the standard to allow 9 digits for instant string offsets.

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #2621 (90f0ab2) into main (3bf0cfe) will increase coverage by 0.00%.
The diff coverage is 98.27%.

@@           Coverage Diff           @@
##             main    #2621   +/-   ##
=======================================
  Coverage   95.98%   95.98%           
=======================================
  Files          20       20           
  Lines       11572    11574    +2     
  Branches     2198     2201    +3     
=======================================
+ Hits        11107    11109    +2     
  Misses        401      401           
  Partials       64       64           
Impacted Files Coverage Δ
polyfill/lib/intl.mjs 97.84% <0.00%> (ø)
polyfill/lib/ecmascript.mjs 98.26% <100.00%> (+<0.01%) ⬆️
polyfill/lib/regex.mjs 100.00% <100.00%> (ø)
polyfill/lib/timezone.mjs 100.00% <100.00%> (ø)
polyfill/lib/zoneddatetime.mjs 100.00% <100.00%> (ø)

spec/abstractops.html Show resolved Hide resolved
spec/abstractops.html Outdated Show resolved Hide resolved
spec/abstractops.html Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
This commit refactors spec text and polyfill code for time zone offsets,
especially to split the handling of offsets in ISO strings from offsets
used as time zone identifiers.

This will help prepare for a later normative commit where time zone
identifiers are limited to minutes precision while ISO string offset
inputs and ZonedDateTime's `offset` property still support nanosecond
precision.
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.

2 participants