Updating calendar-draft.md with recent data model conclusions#487
Conversation
Codecov Report
@@ Coverage Diff @@
## main #487 +/- ##
==========================================
- Coverage 96.59% 96.58% -0.02%
==========================================
Files 17 17
Lines 3705 3694 -11
Branches 558 558
==========================================
- Hits 3579 3568 -11
Misses 124 124
Partials 2 2
Continue to review full report at Codecov.
|
littledan
left a comment
There was a problem hiding this comment.
Overall LGTM; no disagreement about content, and from my perspective, you could land as is. I have some suggestions to improve clarity. Thanks for making these improvements.
docs/calendar-draft.md
Outdated
|
|
||
| Main issue: https://github.com/tc39/proposal-temporal/issues/403 | ||
|
|
||
| If properties of Temporal.Date, etc., are to be enumerable, the calendar should choose which properties to expose; for example, the Japanese calendar might choose to make `.era` enumerable, whereas other calendars might not. This operation can cake place in the factory methods of the Temporal.Calendar protocol, such as `.dateFromFields()`. |
There was a problem hiding this comment.
The explanation of why "era" should be enumerable is not reflected in the issue. Could you summarize this in an issue comment? (Also another part of the issue is whether other fields should be enumerable--IMO we should follow normal JS conventions and make none of them enumerable; I've expressed this in the issue.)
I want to suggest that we simply link to this issue, and work on improving the content/discussion there, rather than summarizing it here, as we may get out of date/sync quickly, and it's hard to follow that issue.
There was a problem hiding this comment.
I removed the aside about era and moved it to the issue.
| This doc describes a design for first-class support for non-Gregorian [calendars](https://en.wikipedia.org/wiki/Calendar) in Temporal. Although most of this document is based on Temporal.Date, most of this applies to Temporal.DateTime, Temporal.YearMonth, Temporal.MonthDay, and Temporal.Time as well. | ||
|
|
||
| ## Temporal.Date internal slots | ||
| ## Data Model |
There was a problem hiding this comment.
It would be good to describe what you mean here by IsoYear, etc; I think this might be confusing for people.
There was a problem hiding this comment.
Added another paragraph
| - `[[IsoYear]]` | ||
| - `[[IsoMonth]]` | ||
| - `[[Calendar]]` | ||
| - `[[RefIsoDay]]` |
There was a problem hiding this comment.
"Ref" would also be good to explain.
|
|
||
| For calendars that use ISO-style months, such as Gregorian, Solar Buddhist, and Japanese, "RefIsoDay" and "RefIsoYear" can be ignored. However, for lunar and lunisolar calendars, such as Hebrew, Saudi Arabian Islamic, and Chinese, these additional fields allow those calendars to disambiguate which YearMonth and MonthDay are being represented. | ||
|
|
||
| ## Temporal.Calendar interface |
There was a problem hiding this comment.
Can we also note here that there's a concrete class, Temporal.Calendar, with a constructor that takes a calendar ID (a la 402), and that you can subclass this class (as described in #300)?
There was a problem hiding this comment.
Does this look correct?
Co-Authored-By: Philip Chimento <philip.chimento@gmail.com>
|
I know its already merged but looks good! |
Marking the following issues fixed, since their consensuses are now documented in the Markdown file:
In addition, the following issues are now superseded: