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

Tighten ISO parsing of month and day values #1836

Merged
merged 1 commit into from Sep 20, 2021

Conversation

facugaich
Copy link
Contributor

Per the spec:

 DateMonth :
      0 NonzeroDigit
      10
      11
      12

 DateDay :
      0 NonzeroDigit
      1 Digit
      2 Digit
      30
      31

The following will now throw RangeError

Temporal.Instant.from("1976-40-18T00:00:00Z");
Temporal.Instant.from("1976-11-50T00:00:00Z");
Temporal.PlainDateTime.from("1976-40-18T00:00:00");
Temporal.PlainDateTime.from("1976-11-50T00:00:00");
etc

Note that yearmonth and monthday were changed for consistency but Temporal.PlainYearMonth.from and Temporal.PlainMonthDay.from already validate these values at a higher level

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks, this is correct!

Do take note that this polyfill code is now deprecated and only used to validate the test262 tests, so this change will never be released; additionally we are in the process of converting test files such as regex.mjs to test262 tests.

So do also consider checking with whatever implementation or polyfill you are using whether they handle this correctly.

@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #1836 (b2f82c8) into main (8b90758) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1836   +/-   ##
=======================================
  Coverage   94.79%   94.79%           
=======================================
  Files          19       19           
  Lines       10963    10967    +4     
  Branches     1713     1713           
=======================================
+ Hits        10392    10396    +4     
  Misses        556      556           
  Partials       15       15           
Flag Coverage Δ
test262 81.18% <100.00%> (+<0.01%) ⬆️
tests 89.54% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/regex.mjs 100.00% <100.00%> (ø)

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 8b90758...b2f82c8. Read the comment docs.

@ptomato ptomato added the no-spec-text PR can be ignored by implementors label Sep 20, 2021
@ptomato ptomato merged commit 180ad10 into tc39:main Sep 20, 2021
@facugaich
Copy link
Contributor Author

So do also consider checking with whatever implementation or polyfill you are using whether they handle this correctly.

Thanks for reviewing and merging, I'll look into doing that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants