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

Normative: Make PlainDate difference methods units handling consistent #1955

Merged
merged 1 commit into from Dec 17, 2021

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Dec 3, 2021

To bring the default for largestUnit in PlainDate's since() and until()
methods in line with other types' since() and until() methods, we have to
add an algorithm step for LargerOfTwoTemporalUnits.

Without this, code such as
date1.until(date2, { smallestUnit: 'months' })
would throw because the default largestUnit is days. As per
#827 (comment)
this was not intended.

PlainDate seems to be the only place where this was not working as
intended. ZonedDateTime, Instant, PlainTime, PlainYearMonth, and
PlainDateTime either already have this step or their default largestUnit
is already the largest one so they wouldn't have this problem.

The reference polyfill code is already correct in this regard.

Closes: #1864

To bring the default for largestUnit in PlainDate's since() and until()
methods in line with other types' since() and until() methods, we have to
add an algorithm step for LargerOfTwoTemporalUnits.

Without this, code such as
date1.until(date2, { smallestUnit: 'months' })
would throw because the default largestUnit is days. As per
#827 (comment)
this was not intended.

PlainDate seems to be the only place where this was not working as
intended. ZonedDateTime, Instant, PlainTime, PlainYearMonth, and
PlainDateTime either already have this step or their default largestUnit
is already the largest one so they wouldn't have this problem.

The reference polyfill code is already correct in this regard.

Closes: #1864
@ptomato ptomato added spec-text Specification text involved needs plenary input Needs to be presented to the committee and feedback incorporated labels Dec 3, 2021
@ptomato ptomato marked this pull request as draft December 3, 2021 21:31
@ptomato
Copy link
Collaborator Author

ptomato commented Dec 3, 2021

Draft until presented at TC39.

cc @FrankYFTang

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #1955 (1e29ab8) into main (c49eb19) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1955   +/-   ##
=======================================
  Coverage   95.00%   95.00%           
=======================================
  Files          19       19           
  Lines       10949    10949           
  Branches     1739     1739           
=======================================
  Hits        10402    10402           
  Misses        531      531           
  Partials       16       16           
Flag Coverage Δ
test262 80.05% <ø> (ø)
tests 89.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out 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 c49eb19...1e29ab8. Read the comment docs.

@ptomato
Copy link
Collaborator Author

ptomato commented Dec 17, 2021

This achieved consensus at the December 2021 TC39 meeting.

@ptomato ptomato marked this pull request as ready for review December 17, 2021 18:40
@ptomato ptomato merged commit 8b7ba00 into main Dec 17, 2021
@ptomato ptomato deleted the 1864-plaindate-units-handling branch December 17, 2021 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs plenary input Needs to be presented to the committee and feedback incorporated spec-text Specification text involved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem in Temporal.PlainDate.prototype.since/until
3 participants