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

'era' belongs in the domain of ECMA-402 #1055

Merged
merged 2 commits into from
Oct 28, 2020
Merged

'era' belongs in the domain of ECMA-402 #1055

merged 2 commits into from
Oct 28, 2020

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Oct 24, 2020

262-compliant implementations should not need the .era and .yearType getters on the Date/DateTime/YearMonth prototypes. However, 402-compliant implementations should add those forwarding getters to the prototypes.

Closes: #1046

@ptomato ptomato marked this pull request as draft October 24, 2020 20:24
@codecov
Copy link

codecov bot commented Oct 24, 2020 •
edited
Loading

Codecov Report

Merging #1055 into main will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1055      +/-   ##
==========================================
+ Coverage   94.53%   94.60%   +0.07%     
==========================================
  Files          18       18              
  Lines        6768     6764       -4     
  Branches     1124     1120       -4     
==========================================
+ Hits         6398     6399       +1     
+ Misses        362      358       -4     
+ Partials        8        7       -1
Flag Coverage Δ
#test262 47.66% <55.55%> (+0.03%) ⬆️
#tests 90.77% <100.00%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
polyfill/lib/date.mjs 91.86% <ø> (-0.10%) ⬇️
polyfill/lib/datetime.mjs 94.94% <ø> (-0.03%) ⬇️
polyfill/lib/yearmonth.mjs 95.03% <ø> (-0.07%) ⬇️
polyfill/lib/calendar.mjs 83.61% <100.00%> (+1.31%) ⬆️
polyfill/lib/intrinsicclass.mjs 59.09% <100.00%> (+0.47%) ⬆️

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 e06c4c3...e03c0a7. Read the comment docs.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 24, 2020

Sorry for the noise, I didn't mean to file this one yet. It needs #901 to go in first.

@sffc
Copy link
Collaborator

sffc commented Oct 26, 2020

Thanks for the change. I'm not sure how this should look in the spec. I guess that's a general question about how we go about specing the 402 calendars.

Ms2ger
Ms2ger requested changes Oct 26, 2020
View reviewed changes
Copy link
Collaborator

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold off on landing this until we have replacement spec text in the intl section.

@ptomato ptomato force-pushed the 1046-era-not-part-of-main-spec branch from 73b49de to cb036ae Compare October 26, 2020 15:50
@ptomato ptomato marked this pull request as ready for review October 27, 2020 14:16
@ptomato
Define %Temporal.Foo.prototype% intrinsics
e9033ec 
These intrinsics are defined in the spec, but were not yet needed in the
polyfill, so hadn't been defined. They will be needed in order for the
ECMA-402 calendars to define custom calendar properties on
%Temporal.Date.prototype%, for example.
@ptomato
Remove era from main spec
e03c0a7 
This removes era from Temporal proper, and defines it only the first time
a calendar that uses it is constructed. This is in line with era and other
custom calendar properties being the domain of ECMA-402.

Closes: #1046
@ptomato ptomato force-pushed the 1046-era-not-part-of-main-spec branch from cb036ae to e03c0a7 Compare October 27, 2020 14:17
@ptomato
Copy link
Collaborator Author

ptomato commented Oct 27, 2020

What would need to change in order to land this? If it's full specs for the calendars in the Intl section, then I'm not sure I agree — that may take some time, and I wouldn't want to keep the incorrect era properties in the main spec when we give it to the reviewers.

@Ms2ger Ms2ger merged commit a7322a6 into main Oct 28, 2020
@Ms2ger Ms2ger deleted the 1046-era-not-part-of-main-spec branch October 28, 2020 09:24
@littledan
Copy link
Member

I think we should revert this change, since it was based on a misunderstanding of #1046. #1046 is all about non-402 environments, but the polyfill and docs do support 402.

@littledan littledan mentioned this pull request Nov 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ms2ger Ms2ger Ms2ger approved these changes

@justingrant justingrant Awaiting requested review from justingrant

@pipobscure pipobscure Awaiting requested review from pipobscure

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@sffc sffc Awaiting requested review from sffc

@gibson042 gibson042 Awaiting requested review from gibson042

@mattjohnsonpint mattjohnsonpint Awaiting requested review from mattjohnsonpint

@ryzokuken ryzokuken Awaiting requested review from ryzokuken

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Era and yearType should be extensions defined in 402
4 participants
@ptomato @sffc @littledan @Ms2ger