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: Fix undefined variables in HandleDateTimeValue #1687

Merged
merged 1 commit into from Sep 2, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Aug 4, 2021

In the PlainDate, PlainDateTime, PlainMonthDay, PlainTime, and
PlainYearMonth cases, the timeZone variable was never set before it was
used. This is an error in the spec text. It should be equal to the
time zone in the DateTimeFormat's resolvedOptions.

Add tests proving that this time zone is used when converting the Plain
objects to exact time, so that the wall time in the same time zone is used
when formatting them.

In the PlainDate, PlainDateTime, PlainMonthDay, PlainTime, and
PlainYearMonth cases, the _timeZone_ variable was never set before it was
used. This is an error in the spec text. It should be equal to the
time zone in the DateTimeFormat's resolvedOptions.

Add tests proving that this time zone is used when converting the Plain
objects to exact time, so that the wall time in the same time zone is used
when formatting them.
@ptomato ptomato added the needs plenary input Needs to be presented to the committee and feedback incorporated label Aug 4, 2021
@ptomato ptomato marked this pull request as draft August 4, 2021 23:10
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 4, 2021

Converting to draft so that the normative change is not accidentally merged.

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #1687 (f86404c) into main (d865d71) will decrease coverage by 3.93%.
The diff coverage is n/a.

❗ Current head f86404c differs from pull request most recent head 3ceeb2f. Consider uploading reports for the commit 3ceeb2f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1687      +/-   ##
==========================================
- Coverage   95.03%   91.09%   -3.94%     
==========================================
  Files          19       17       -2     
  Lines       10785    10774      -11     
  Branches     1725     1608     -117     
==========================================
- Hits        10249     9815     -434     
- Misses        523      945     +422     
- Partials       13       14       +1     
Flag Coverage Δ
test262 ?
tests 90.15% <ø> (ø)

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

Impacted Files Coverage Δ
polyfill/lib/plaindate.mjs 72.06% <0.00%> (-18.31%) ⬇️
polyfill/lib/plaintime.mjs 79.72% <0.00%> (-11.03%) ⬇️
polyfill/lib/plainmonthday.mjs 83.56% <0.00%> (-9.59%) ⬇️
polyfill/lib/zoneddatetime.mjs 90.62% <0.00%> (-7.77%) ⬇️
polyfill/lib/instant.mjs 87.15% <0.00%> (-7.30%) ⬇️
polyfill/lib/timezone.mjs 86.27% <0.00%> (-7.19%) ⬇️
polyfill/lib/plaindatetime.mjs 90.34% <0.00%> (-6.97%) ⬇️
polyfill/lib/plainyearmonth.mjs 89.77% <0.00%> (-6.63%) ⬇️
polyfill/lib/duration.mjs 93.65% <0.00%> (-4.43%) ⬇️
polyfill/lib/calendar.mjs 91.40% <0.00%> (-2.97%) ⬇️
... and 5 more

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 d865d71...3ceeb2f. Read the comment docs.

@ptomato ptomato marked this pull request as ready for review August 31, 2021 19:23
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 31, 2021

This change achieved consensus in TC39.

@Ms2ger Mind taking a final look?

@Ms2ger Ms2ger merged commit c0786b7 into main Sep 2, 2021
@Ms2ger Ms2ger deleted the fix-undefined-variables branch September 2, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs plenary input Needs to be presented to the committee and feedback incorporated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants