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: Consistent order of operations in PMD.toPD and PYM.toPD #1734

Merged
merged 1 commit into from Sep 2, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Aug 16, 2021

The spec text was inconsistent about whether the type check on the
argument or the calendar.fields checks should occur first. In
Temporal.PlainMonthDay.prototype.toPlainDate, the fields checks were
first. In Temporal.PlainYearMonth.prototype.toPlainDate, the type check
was first. This changes the PlainMonthDay algorithm to match the
PlainYearMonth one, and adds tests to both.

Closes: #1702

@ptomato ptomato requested a review from Ms2ger August 16, 2021 20:09
@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #1734 (17dbd03) into main (e780f7c) will decrease coverage by 3.75%.
The diff coverage is 100.00%.

❗ Current head 17dbd03 differs from pull request most recent head 1a99004. Consider uploading reports for the commit 1a99004 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1734      +/-   ##
==========================================
- Coverage   94.85%   91.10%   -3.76%     
==========================================
  Files          19       17       -2     
  Lines       10882    10776     -106     
  Branches     1729     1610     -119     
==========================================
- Hits        10322     9817     -505     
- Misses        547      945     +398     
- Partials       13       14       +1     
Flag Coverage Δ
test262 ?
tests 90.14% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/plainmonthday.mjs 83.67% <100.00%> (-10.21%) ⬇️
polyfill/lib/plainyearmonth.mjs 89.80% <100.00%> (-6.62%) ⬇️
polyfill/lib/plaindate.mjs 72.06% <0.00%> (-18.34%) ⬇️
polyfill/lib/plaintime.mjs 79.72% <0.00%> (-9.11%) ⬇️
polyfill/lib/zoneddatetime.mjs 90.62% <0.00%> (-7.84%) ⬇️
polyfill/lib/timezone.mjs 86.27% <0.00%> (-7.19%) ⬇️
polyfill/lib/plaindatetime.mjs 90.34% <0.00%> (-6.97%) ⬇️
polyfill/lib/instant.mjs 87.15% <0.00%> (-6.95%) ⬇️
... and 7 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 e780f7c...1a99004. Read the comment docs.

@ptomato
Copy link
Collaborator Author

ptomato commented Aug 16, 2021

Draft, in order to avoid accidentally merging before presenting for consensus

@ptomato ptomato marked this pull request as draft August 16, 2021 20:11
@ptomato ptomato marked this pull request as ready for review August 31, 2021 18:13
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 31, 2021

This change achieved consensus in TC39. @Ms2ger would you be able to take a final look?

The spec text was inconsistent about whether the type check on the
argument or the calendar.fields checks should occur first. In
Temporal.PlainMonthDay.prototype.toPlainDate, the fields checks were
first. In Temporal.PlainYearMonth.prototype.toPlainDate, the type check
was first. This changes the PlainMonthDay algorithm to match the
PlainYearMonth one, and adds tests to both.
@Ms2ger Ms2ger force-pushed the 1702-plainmonthday-toplaindate-type-check branch from 0af998c to 1a99004 Compare September 2, 2021 12:42
@Ms2ger Ms2ger enabled auto-merge (rebase) September 2, 2021 12:53
@Ms2ger Ms2ger merged commit dd1e535 into main Sep 2, 2021
@Ms2ger Ms2ger deleted the 1702-plainmonthday-toplaindate-type-check branch September 2, 2021 13:06
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.

Order of If Type(item) is not Object, throw in PlainMonthDay and PlainYearMonth toPlainDate
3 participants