Normative: Merge PrepareTemporalFields #2472
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2472 +/- ##
==========================================
+ Coverage 95.47% 95.50% +0.02%
==========================================
Files 20 20
Lines 10929 10931 +2
Branches 2034 2035 +1
==========================================
+ Hits 10435 10440 +5
+ Misses 432 430 -2
+ Partials 62 61 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
spec/abstractops.html
Outdated
| 1. If _requiredFields_ is not ~partial~, then | ||
| 1. Let _era_ be ? Get(_result_, *"era"*). | ||
| 1. Let _eraYear_ be ? Get(_result_, *"eraYear"*). | ||
| 1. If _era_ is *undefined* and _eraYear_ is not *undefined*, throw a *RangeError* exception. | ||
| 1. If _era_ is not *undefined* and _eraYear_ is *undefined*, throw a *RangeError* exception. |
There was a problem hiding this comment.
As per #2465 (comment) we'll remove this part because it is already done in dateFromFields/yearMonthFromFields/monthDayFromFields anyway, after each call to PrepareTemporalFields.
There was a problem hiding this comment.
Is there a harm in validating it here as well?
There was a problem hiding this comment.
Yes, it "leaks" calendar-specific logic outside of Temporal.Calendar methods.
There was a problem hiding this comment.
Is there likely to be any calendar that supports one of these two properties but not both?
There was a problem hiding this comment.
No. Calendars either use eras or they won't. If they do use eras (which is, BTW, the vast majority of CLDR calendars) then era/eraYear are used as a pair. One of then without the other is not meaningful.
This means that discussions relating to validation and eras are focused on relatively narrow questions:
- In non-era-using calendars, or in 262 without 402, what should happen if callers provide (in a property bag passed to
from,with, etc.) one ofera/eraYearwithout the other? Throw? Ignore? - Where exactly should the validation take place that ensures that those two properties are either both present or both
undefined?
There was a problem hiding this comment.
I think that unknown properties should be ignored, but known ones should be validated as early as possible.
Given that these two properties are really just one property, that contains two items, and should only come as a pair, it seems reasonable to me to validate for them here as well, since they're known property names.
There was a problem hiding this comment.
That is validated already, this just removes a duplicate validation. I get what you are saying about validating as early as possible, but I don't think that weighs against having calendar-specific logic live outside of Temporal.Calendar methods.
There was a problem hiding this comment.
This AO is called "PrepareTemporalFields" and Calendar is part of Temporal - since this is logic that would apply to any era-using calendar, it doesn't feel calendar-specific to me, it feels calendar-general, which doesn't imo need to be contained to calendar methods.
|
This achieved consensus at the 2021-01-31 TC39 plenary meeting. @ljharb, I assume your open question was resolved by the question you asked during the presentation about I think before merging this we should have a test262 test to verify it. Something based on the code sample I gave in my slides (https://ptomato.name/talks/tc39-2023-01/#12) would be sufficient, I think; that code sample should work for the ISO calendar regardless of whether the implementation has other calendars or just ISO. |
|
@ptomato yes, as long as ecma262 has zero mention of era/eraYear, and all of it is in ecma402, then I can tolerate it :-) (altho my preference would be for everything to be in 262, and for 402 to only patch methods or AOs as needed to add intl-specific logic). |
Aditi-1400
force-pushed
the
mergePrepareTemporalFields
branch
2 times, most recently
from
b9e6313
to
ee0cf92
Compare
Avoiding two different version of PrepareTemporalFields for unneeded reason by adding the era and eraYear check into the ECAM262 version and remove the ECMA402 version.
Aditi-1400
force-pushed
the
mergePrepareTemporalFields
branch
from
ee0cf92
to
4dff8ed
Compare
|
Tests are in: tc39/test262#3792 |
Aditi-1400
force-pushed
the
mergePrepareTemporalFields
branch
from
4dff8ed
to
e4b2001
Compare
ptomato
force-pushed
the
mergePrepareTemporalFields
branch
from
e4b2001
to
d8c8bb1
Compare
|
Thanks, merging now! |
Avoiding two different versions of PrepareTemporalFields for unneeded reason by adding the 'era' and 'eraYear' check into the ECAM262 version and remove the ECMA402 version.
Close #2465