-
Notifications
You must be signed in to change notification settings - Fork 154
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
Polyfill: fix codecov/Test262 issues in ISO parser & align it closer to spec #2635
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2635 +/- ##
==========================================
- Coverage 96.05% 95.98% -0.08%
==========================================
Files 20 20
Lines 11563 11533 -30
Branches 2204 2184 -20
==========================================
- Hits 11107 11070 -37
- Misses 392 399 +7
Partials 64 64
|
Grrrrr, validStrings.mjs has an expectation that the resulting offset is in normalized format. Converting to draft until I can figure out how to solve that. |
dac9225
to
49bee7d
Compare
Fixed now. Ready for review. |
To make it easier to find actual Test262 gaps, this PR removes unreachable and unused code in ISO parsing. It also removes offset normalization code from ParseISODateTime, to better mach the spec that only normalizes downstream from ISO string parsing. Doing this exposed a test bug that's fixed in the second commit of tc39/test262#3877.
49bee7d
to
5f9e901
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a little bit to figure out, but there are two things going on here. I'll note them for posterity:
- Handling
offsetFraction
in ParseISODateTime was incorrect. - ParseISODateTime didn't need to have the offset regexp with capture groups anyway.
Thanks!
Interestingly, this PR accidentally fixed another polyfill bug that @anba found. Previously, Temporal.TimeZone.from("0000-01-01T00:00+00:00:00") didn't throw because ParseISODateTime normalized trailing second and sub-second zeroes. Now that this PR's ParseISODateTime returns the offset string as-is, ParseDateTimeUTCOffset can detect sub-second offsets and throw when they're present, even if they are all zeroes. |
To make it easier to find Test262 gaps, this PR removes unreachable code in ISO parsing in the polyfill that codecov complained about.
It also removes offset normalization code from ParseISODateTime, to better mach the spec that only normalizes downstream from ISO string parsing. Doing this exposed a test bug that's fixed in the second commit of tc39/test262#3877. For good measure, this PR pulls in the entirety of tc39/test262#3877.
Note that this PR shouldn't be merged until the corresponding Test262 PR is merged.