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

Spec: Throw on undefined calendar in Temporal.now #1391

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

cjtenny
Copy link
Collaborator

@cjtenny cjtenny commented Feb 26, 2021

I found another spec discrepancy while auditing docs code samples, where the docs+polyfill disagree with spec.

Temporal.now.{plainDate,plainDateTime,zonedDateTime}() should accept undefined calendar because they all delegate to SystemDateTime which uses ToOptionalTemporalCalendar. However, calendarLike isn't listed as an optional argument.

Which is the correct behavior? Assuming the spec is correct, this makes the polyfill match and adds some optionality-brackets to the spec for clarity.

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #1391 (f3ae891) into main (824923a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1391   +/-   ##
=======================================
  Coverage   95.36%   95.37%           
=======================================
  Files          19       19           
  Lines       11137    11154   +17     
  Branches     1829     1832    +3     
=======================================
+ Hits        10621    10638   +17     
  Misses        511      511           
  Partials        5        5           
Flag Coverage Δ
test262 48.34% <0.00%> (-0.05%) ⬇️
tests 91.09% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/duration.mjs 98.29% <100.00%> (+<0.01%) ⬆️
polyfill/lib/ecmascript.mjs 96.10% <100.00%> (+0.01%) ⬆️

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 824923a...c44fa15. Read the comment docs.

@cjtenny
Copy link
Collaborator Author

cjtenny commented Feb 26, 2021

Should adjust #1392 to match if allowing undefined calendars is what we want; otherwise I'll redo this change to fix the spec to match the polyfill + docs.

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.

The polyfill and docs are actually correct here, it's a bug in the spec text. (This was probably the one single Temporal issue of the past year on which the most time was spent, so we don't want to get it wrong!)

@cjtenny cjtenny changed the title Polyfill: Handle undefined calendar for Temporal.now methods Spec: Throw on undefined calendar in Temporal.now Feb 26, 2021
@cjtenny
Copy link
Collaborator Author

cjtenny commented Feb 26, 2021

Got it. Changed to reflect that!

@Ms2ger Ms2ger merged commit 4b3e7fa into main Feb 26, 2021
@Ms2ger Ms2ger deleted the optional-calendars-now branch February 26, 2021 07:51
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.

3 participants