Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Normative: Reject non-integer Duration fields in Duration.with() #1735

Merged
merged 2 commits into from
Sep 2, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Aug 16, 2021

Unintentionally forgot with() when rejecting non-integer Duration fields
in issue #938.

Adds tests for anywhere Duration property bags are accepted, fixes a test
that would fail under the new spec text, and brings the testing polyfill
code in alignment with the new spec text.

Closes: #1704

@ptomato ptomato requested a review from Ms2ger August 16, 2021 21:14
@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #1735 (018e96d) into main (e780f7c) will decrease coverage by 3.76%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #1735      +/-   ##
==========================================
- Coverage   94.85%   91.08%   -3.77%     
==========================================
  Files          19       17       -2     
  Lines       10882    10772     -110     
  Branches     1729     1606     -123     
==========================================
- Hits        10322     9812     -510     
- Misses        547      946     +399     
- Partials       13       14       +1     
Flag Coverage Δ
test262 ?
tests 90.13% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 94.49% <100.00%> (-0.37%) ⬇️
polyfill/lib/plaindate.mjs 72.06% <0.00%> (-18.34%) ⬇️
polyfill/lib/plainmonthday.mjs 83.56% <0.00%> (-10.32%) ⬇️
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%) ⬇️
polyfill/lib/plainyearmonth.mjs 89.77% <0.00%> (-6.64%) ⬇️
polyfill/lib/duration.mjs 93.65% <0.00%> (-4.43%) ⬇️
... and 6 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...b0b41d9. Read the comment docs.

@ptomato ptomato force-pushed the 1704-duration-with-noninteger branch from 73eb281 to 463a9f0 Compare August 16, 2021 22:12
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 16, 2021

(Draft because normative PR waiting for consensus)

@ptomato ptomato marked this pull request as draft August 16, 2021 23:33
@ptomato ptomato marked this pull request as ready for review August 31, 2021 18:37
@ptomato ptomato force-pushed the 1704-duration-with-noninteger branch from 463a9f0 to c85bedb Compare August 31, 2021 18:42
@ptomato
Copy link
Collaborator Author

ptomato commented Aug 31, 2021

This change achieved consensus in TC39.

Rebased and fixed some conflicts; @Ms2ger would you mind taking a final look?

Unintentionally forgot with() when rejecting non-integer Duration fields
in issue #938.

Adds tests for anywhere Duration property bags are accepted, fixes a test
that would fail under the new spec text, and brings the testing polyfill
code in alignment with the new spec text.

Closes: #1704
…onRecord

The test should be floor(abs(v)) == abs(v). Use the existing abstract
operation. Adds an extra line to each test which should cover this as
well.
@Ms2ger Ms2ger force-pushed the 1704-duration-with-noninteger branch from c85bedb to b0b41d9 Compare September 2, 2021 13:11
@Ms2ger Ms2ger enabled auto-merge (rebase) September 2, 2021 13:11
@Ms2ger Ms2ger merged commit fa9d547 into main Sep 2, 2021
@Ms2ger Ms2ger deleted the 1704-duration-with-noninteger branch September 2, 2021 13:40
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-integer handling is inconsistent between ToTemporalDurationRecord and ToPartialDuration
3 participants