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 TimeZone [[OffsetNanoseconds]] internal slot to [[OffsetMinutes]] #2622

Merged

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 28, 2023

This PR is stacked on #2607. Please review only the last commit. At @ptomato's request, I'm splitting it into a separate editorial PR to simplify review of the other normative PR. It's marked as a draft until #2607 is approved at the upcoming TC39 meeting.

Now that #2607 limits TimeZone's [[OffsetNanoseconds]] internal slot to minute precision, this PR refactors TimeZone to clarify that only minutes are allowed in that slot and related abstract operations.

Changes:

  • Renames TimeZone's [[OffsetNanoseconds]] internal slot to [[OffsetMinutes]]
  • Changes ParseTimeZoneIdentifier to return an [[OffsetMinutes]] field instead of an [[OffsetNanoseconds]] field.
  • Changes FormatOffsetTimeZoneIdentifier to expect its argument to be an integer number of minutes, not an integer number of nanoseconds that's required to be evenly divisible by 6e10.

The goal of this change is to avoid the complexity and potential confusion from a slot and AOs that deal with "nanoseconds" values that nonetheless are restricted to minutes.

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #2622 (890327c) into main (c429eed) will decrease coverage by 0.02%.
The diff coverage is 96.59%.

@@            Coverage Diff             @@
##             main    #2622      +/-   ##
==========================================
- Coverage   95.96%   95.94%   -0.02%     
==========================================
  Files          20       20              
  Lines       11528    11550      +22     
  Branches     2193     2197       +4     
==========================================
+ Hits        11063    11082      +19     
- Misses        401      404       +3     
  Partials       64       64              
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 98.21% <96.00%> (-0.05%) ⬇️
polyfill/lib/calendar.mjs 86.84% <100.00%> (-0.04%) ⬇️
polyfill/lib/regex.mjs 100.00% <100.00%> (ø)
polyfill/lib/timezone.mjs 100.00% <100.00%> (ø)

@justingrant justingrant force-pushed the timezone-slot-refactor-to-minutes branch 4 times, most recently from 6a57343 to 49ecd37 Compare June 29, 2023 05:35
@justingrant
Copy link
Collaborator Author

In today's TG2 meeting, there was confusion from folks who saw the [[OffsetNanoseconds]] in spec text and assumed that it meant that full range of nanoseconds was supported, even though it won't be thanks to #2607. Clarifying that nanosecond precision was not going to be supported was randomizing in the meeting and used up valuable time I would have preferred to spend elsewhere. Had this PR's changes already been merged, this would have been a non-issue.

Anyway, that meeting cemented my opinion that we should make this renaming editorial change, because it makes the allowed precision clearer for spec readers or reviewers who aren't as familiar with the rest of Temporal.

@justingrant justingrant force-pushed the timezone-slot-refactor-to-minutes branch 2 times, most recently from 4bf9fad to 100118a Compare July 3, 2023 17:44
@justingrant justingrant force-pushed the timezone-slot-refactor-to-minutes branch 3 times, most recently from 2d76d40 to d3c20e7 Compare July 18, 2023 15:40
@justingrant justingrant marked this pull request as ready for review July 18, 2023 15:43
@justingrant justingrant force-pushed the timezone-slot-refactor-to-minutes branch from d3c20e7 to 5eca86f Compare July 18, 2023 15:58
Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

I still prefer keeping everything in nanoseconds, but I understand the motivation here.

spec/timezone.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/timezone.html Outdated Show resolved Hide resolved
spec/mainadditions.html Outdated Show resolved Hide resolved
@justingrant justingrant force-pushed the timezone-slot-refactor-to-minutes branch 5 times, most recently from 5e6de69 to 4ed1f37 Compare July 19, 2023 01:05
Now that we've limited TimeZone's [[OffsetNanoseconds]] internal slot
to minute precision, this commit refactors TimeZone to clarify that only
minutes are allowed in that slot and related abstract operations.

Changes:
* Renames TimeZone's [[OffsetNanoseconds]] internal slot to
  [[OffsetMinutes]]
* Changes ParseTimeZoneIdentifier to return an [[OffsetMinutes]] field
  instead of an [[OffsetNanoseconds]] field.
* Changes FormatOffsetTimeZoneIdentifier to expect a minutes argument.

The goal of this change is to avoid the complexity and potential
confusion from a slot and AOs that deal with "nanoseconds" values
that nonetheless are restricted to minutes.
@justingrant justingrant force-pushed the timezone-slot-refactor-to-minutes branch from 4ed1f37 to 890327c Compare July 19, 2023 04:12
@justingrant justingrant merged commit 6834487 into tc39:main Jul 19, 2023
9 checks passed
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