-
Notifications
You must be signed in to change notification settings - Fork 145
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
Specify era/eraYear. #1311
Specify era/eraYear. #1311
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1311 +/- ##
==========================================
- Coverage 95.77% 95.61% -0.16%
==========================================
Files 19 19
Lines 9419 9373 -46
Branches 1445 1437 -8
==========================================
- Hits 9021 8962 -59
- Misses 392 405 +13
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1. Perform ? RequireInternalSlot(_calendar_, [[InitializedTemporalCalendar]]). | ||
1. If _calendar_.[[Identifier]] is *"iso8601"*, then | ||
1. Return *undefined*. | ||
1. Let _era_ be the result of implementation-defined processing of _dateOrDateTime_ and the value of _calendar_.[[Identifier]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very hand-wavey. can this be elaborated on, or made less imprecise?
spec/intl.html
Outdated
1. Perform ? RequireInternalSlot(_calendar_, [[InitializedTemporalCalendar]]). | ||
1. If _calendar_.[[Identifier]] is *"iso8601"*, then | ||
1. Return *undefined*. | ||
1. Let _era_ be the result of implementation-defined processing of _dateOrDateTime_ and the value of _calendar_.[[Identifier]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also here
<emu-clause id="sup-temporal.calendar.prototype.erayear"> | ||
<h1>Temporal.Calendar.prototype.eraYear ( _dateOrDateTime_ )</h1> | ||
<p> | ||
The `eraYear` method takes one argument _dateOrDateTime_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `eraYear` method takes one argument _dateOrDateTime_. | |
The `eraYear` method takes one argument, _dateOrDateTime_. |
What kind of argument?
<emu-clause id="sup-temporal.calendar.prototype.era"> | ||
<h1>Temporal.Calendar.prototype.era ( _dateOrDateTime_ )</h1> | ||
<p> | ||
The `era` method takes one argument _dateOrDateTime_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `era` method takes one argument _dateOrDateTime_. | |
The `era` method takes one argument, _dateOrDateTime_. |
What kind of argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to do about the vagueness of the "implementation-defined processing" bits. We can't really specify out all the ICU calendars, but on the other hand I do take the point that it is weird to have methods that return either undefined
or an implementation-defined result with no explanation.
Would that situation be sufficiently improved when we add the corresponding explanation in the documentation, or does there need to be some explanation in the spec text as well?
I think it needs to be clear in the spec steps. |
Fixes #1308.
Editorial improvements are still welcome, of course, but I've merged this in the interest of stabilizing the normative parts of the spec. |
Given there’s still a number of open review comments, I’m not sure why merging prior to addressing them is stabilizing. |
Fixes #1308.