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.

Sign up for GitHub

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

Jump to bottom

compare methods should not include calendar #1457

Merged
merged 3 commits into from
Mar 23, 2021
Merged

compare methods should not include calendar #1457

merged 3 commits into from
Mar 23, 2021

Conversation

ryzokuken
Copy link
Member

This commit changes different compare methods on types that include
calendars to not take them into account. With this, two instances that
have different calendars attached but map to the same points in the ISO
calendar are compared to be "equal".

Fixes: #1431

/cc @bakkot

@ryzokuken ryzokuken added this to the Stage 3 Conditional milestone Mar 22, 2021
@ryzokuken ryzokuken requested review from ptomato and cjtenny March 22, 2021 04:16
@ryzokuken ryzokuken self-assigned this Mar 22, 2021
@codecov
Copy link

codecov bot commented Mar 22, 2021 •
edited
Loading

Codecov Report

Merging #1457 (3938491) into main (fa38136) will decrease coverage by 46.69%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #1457       +/-   ##
===========================================
- Coverage   95.77%   49.07%   -46.70%     
===========================================
  Files          19       18        -1     
  Lines       11090     4952     -6138     
  Branches     1801     1079      -722     
===========================================
- Hits        10621     2430     -8191     
- Misses        465     2114     +1649     
- Partials        4      408      +404
Flag Coverage Δ
test262 49.07% <50.00%> (+0.03%) ⬆️
tests ?

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

Impacted Files Coverage Δ
polyfill/lib/plaindatetime.mjs 54.31% <0.00%> (-40.57%) ⬇️
polyfill/lib/zoneddatetime.mjs 42.78% <0.00%> (-55.15%) ⬇️
polyfill/lib/plaindate.mjs 49.79% <100.00%> (-46.25%) ⬇️
polyfill/lib/plainyearmonth.mjs 67.13% <100.00%> (-28.21%) ⬇️
polyfill/lib/calendar.mjs 17.11% <0.00%> (-77.32%) ⬇️
polyfill/lib/duration.mjs 53.36% <0.00%> (-44.94%) ⬇️
polyfill/lib/ecmascript.mjs 54.64% <0.00%> (-41.61%) ⬇️
polyfill/lib/plaintime.mjs 55.12% <0.00%> (-41.50%) ⬇️
... and 11 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 fa38136...3938491. Read the comment docs.

@ryzokuken ryzokuken marked this pull request as ready for review March 22, 2021 04:46
bakkot
bakkot reviewed Mar 22, 2021
View reviewed changes
spec/zoneddatetime.html Outdated Show resolved Hide resolved
@ryzokuken
Copy link
Member Author

Undrafting this, this should be good to go after a round of reviews. What should I do about timezones in ZonedDateTime? I suppose the same rationale applies? I did not change it yet since the original issue specifically talked about calendars.

@ryzokuken
Copy link
Member Author

@bakkot ah, beat me to it. I did not remember the exact details of the consensus. Let me push another commit to get rid of the timezones as well!

@ryzokuken
Copy link
Member Author

@bakkot should be fine now, PTAL 😄

ptomato
ptomato approved these changes Mar 22, 2021
View reviewed changes
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.

A minor naming comment, otherwise this looks good.

spec/plaindate.html Outdated Show resolved Hide resolved
polyfill/test/zoneddatetime.mjs Outdated Show resolved Hide resolved
spec/plaindatetime.html Outdated Show resolved Hide resolved
@ryzokuken
spec: compare methods should not include calendar
00e7d72 
This commit changes different compare methods on types that include
calendars to not take them into account. With this, two instances that
have different calendars attached but map to the same points in the ISO
calendar are compared to be "equal".

Fixes: tc39#1431
@ryzokuken
polyfill: compare methods should not include calendar
6203ed5 
This commit updates the polyfill to reflect the spec changes made for
addressing tc39#1431.
@ryzokuken
spec, polyfill: exclude timezone from compare
3938491 
Exclude the timezone component from ZonedDateTime.compare comparisons,
so two instances that correspond to the exact same instant of time in
different timezones will compare to be "equal" by returning 0.
@ptomato ptomato merged commit c69ca8e into tc39:main Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bakkot bakkot bakkot left review comments

@ptomato ptomato ptomato approved these changes

@cjtenny cjtenny Awaiting requested review from cjtenny

Assignees

@ryzokuken ryzokuken

Labels
None yet
Projects
None yet
Milestone
Stage 3 Conditional
Development

Successfully merging this pull request may close these issues.

Date comparisons should not include the calendar
3 participants
@ryzokuken @ptomato @bakkot