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

ISOMonthDayFromFields: Reject leap day in appropriate cases when calendar is specified. #1395

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

cjtenny
Copy link
Collaborator

@cjtenny cjtenny commented Feb 26, 2021

ToTemporalMonthDay accepts 'month' without 'monthCode' or 'year' when no calendar has been specified, for users writing calendar-insensitive code. However, when a calendar has been specified, ISOMonthDayFromFields will still accept 2/29/2001 specified via month and year. This change makes ISOMonthDayFromFields reject or constrain 02-29 when month + year are specified, and keeps acceptance of 02-29 when only monthCode + day are specified. See the added test case for some examples.

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #1395 (d712c55) into main (a540826) will decrease coverage by 47.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1395       +/-   ##
===========================================
- Coverage   95.37%   48.30%   -47.08%     
===========================================
  Files          19       18        -1     
  Lines       11154     5010     -6144     
  Branches     1832     1113      -719     
===========================================
- Hits        10638     2420     -8218     
- Misses        511     2156     +1645     
- Partials        5      434      +429     
Flag Coverage Δ
test262 48.30% <100.00%> (-0.05%) ⬇️
tests ?

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

Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 53.70% <ø> (-42.41%) ⬇️
polyfill/lib/calendar.mjs 16.72% <100.00%> (-77.44%) ⬇️
polyfill/lib/zoneddatetime.mjs 41.74% <0.00%> (-55.39%) ⬇️
polyfill/lib/plaindate.mjs 48.67% <0.00%> (-45.93%) ⬇️
polyfill/lib/duration.mjs 53.36% <0.00%> (-44.94%) ⬇️
polyfill/lib/plaintime.mjs 55.12% <0.00%> (-41.50%) ⬇️
polyfill/lib/plaindatetime.mjs 53.03% <0.00%> (-40.41%) ⬇️
polyfill/lib/intl.mjs 65.89% <0.00%> (-34.11%) ⬇️
... and 9 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 a540826...4d94f15. Read the comment docs.

…ndar is specified.

ToTemporalMonthDay accepts 'month' without 'monthCode' or 'year' when no
calendar has been specified, for users writing calendar-insensitive
code. However, when a calendar has been specified, ISOMonthDayFromFields will
still accept 2/29/2001 specified via month and year. This change makes
ISOMonthDayFromFields reject or constrain 02-29 when month + year are specified,
and keeps acceptance of 02-29 when only monthCode + day are specified. See the
added test case for some examples.
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.

This looks like a correct implementation of what we had discussed! Unfortunate that I didn't get it right the first time, but good that you noticed we can remove all those unused abstract operations.

@ptomato ptomato merged commit 69e73f7 into main Feb 26, 2021
@ptomato ptomato deleted the isoleapday branch February 26, 2021 22:47
cjtenny added a commit that referenced this pull request Feb 27, 2021
Running all the code samples in the docs through a tolerant markdown snippet parser + execution environment revealed many errors. #1395, #1391, #1380, and this patch's one-line fix in plainmonthday.mjs were all bugs revealed by the documentation code samples. Many of the changes in this patch are slight syntactic changes to make the samples testable and more clear.
cjtenny added a commit that referenced this pull request Feb 27, 2021
Running all the code samples in the docs through a tolerant markdown snippet parser + execution environment revealed many errors. #1395, #1391, #1380, and this patch's one-line fix in plainmonthday.mjs were all bugs revealed by the documentation code samples. Many of the changes in this patch are slight syntactic changes to make the samples testable and more clear.

Common classes of smaller issues fixed:
- Examples did not include seconds in string representations
- Examples contained extra zeroes of precision when not requested
- Old calling conventions used; arguments needed to be changed
- Some options bags are now used in dedicated places rather than alongside other methods
cjtenny added a commit that referenced this pull request Apr 8, 2021
Running all the code samples in the docs through a tolerant markdown snippet parser + execution environment revealed many errors. #1395, #1391, #1380, and this patch's one-line fix in plainmonthday.mjs were all bugs revealed by the documentation code samples. Many of the changes in this patch are slight syntactic changes to make the samples testable and more clear.

Common classes of smaller issues fixed:
- Examples did not include seconds in string representations
- Examples contained extra zeroes of precision when not requested
- Old calling conventions used; arguments needed to be changed
- Some options bags are now used in dedicated places rather than alongside other methods
cjtenny added a commit that referenced this pull request Apr 13, 2021
Running all the code samples in the docs through a tolerant markdown snippet parser + execution environment revealed many errors. #1395, #1391, #1380, and this patch's one-line fix in plainmonthday.mjs were all bugs revealed by the documentation code samples. Many of the changes in this patch are slight syntactic changes to make the samples testable and more clear.

Common classes of smaller issues fixed:
- Examples did not include seconds in string representations
- Examples contained extra zeroes of precision when not requested
- Old calling conventions used; arguments needed to be changed
- Some options bags are now used in dedicated places rather than alongside other methods
cjtenny added a commit that referenced this pull request Apr 13, 2021
Running all the code samples in the docs through a tolerant markdown snippet parser + execution environment revealed many errors. #1395, #1391, #1380, and this patch's one-line fix in plainmonthday.mjs were all bugs revealed by the documentation code samples. Many of the changes in this patch are slight syntactic changes to make the samples testable and more clear.

Common classes of smaller issues fixed:
- Examples did not include seconds in string representations
- Examples contained extra zeroes of precision when not requested
- Old calling conventions used; arguments needed to be changed
- Some options bags are now used in dedicated places rather than alongside other methods
cjtenny added a commit that referenced this pull request Apr 13, 2021
Running all the code samples in the docs through a tolerant markdown snippet parser + execution environment revealed many errors. #1395, #1391, #1380, and this patch's one-line fix in plainmonthday.mjs were all bugs revealed by the documentation code samples. Many of the changes in this patch are slight syntactic changes to make the samples testable and more clear.

Common classes of smaller issues fixed:
- Examples did not include seconds in string representations
- Examples contained extra zeroes of precision when not requested
- Old calling conventions used; arguments needed to be changed
- Some options bags are now used in dedicated places rather than alongside other methods
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.

2 participants