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.

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

Document the default, optional-ness, etc. for era and non-ISO calendar fields #902

Closed
justingrant opened this issue Sep 15, 2020 · 2 comments · Fixed by #969
Closed

Document the default, optional-ness, etc. for era and non-ISO calendar fields #902

justingrant opened this issue Sep 15, 2020 · 2 comments · Fixed by #969
Assignees
Labels
documentation Additions to documentation

Comments

@justingrant
Copy link
Collaborator

justingrant commented Sep 15, 2020

The docs for DateTime.from say:

At least the year, month, and day properties must be present. If calendar is missing, it will be assumed to be Temporal.Calendar.from('iso8601'). Any other missing ones will be assumed to be 0.

What about era? What about additional non-ISO calendar fields?

Seems like we should have some more generalized text here, e.g.

Any other missing fields will be set to default values determined by the calendar. For the most-commonly-used iso8601 calendar, these defaults are zero for time fields and "CE" for era. Other calendars may apply different defaults (for example the default hour in the ethiopic calendar is 6) or may require some fields that are optional in the ISO 8601 calendar.

BTW, I have no idea what the default era value should be. See #901.

@ptomato
Copy link
Collaborator

ptomato commented Sep 15, 2020

Hmm, maybe this?

At least the year, month, and day properties must be present. If calendar is one that requires era (such as in the Japanese calendar), then the era property must also be present. If calendar is missing, it will be assumed to be Temporal.Calendar.from('iso8601').

I don't know if we want the default values for the other properties to be determined by the calendar. Although, I guess we have no choice, since 0 is an invalid value for hour in the ethiopic calendar.

@justingrant
Copy link
Collaborator Author

I don't know if we want the default values for the other properties to be determined by the calendar. Although, I guess we have no choice, since 0 is an invalid value for hour in the ethiopic calendar.

Yep. Would this imply a change to CalendarProtocol? Or just a change in how the polyfill is implemented?

Hmm, maybe this?

I would probably split into two paragraphs for clarity: one focused on the calendar, and a follow-up paragraph focused on other fields including defaults. Then you'll have prepared the user (who may not even know what a calendar is) before talking about calendar-dependent info.

If calendar is missing, it will be assumed to be Temporal.Calendar.from('iso8601'). This calendar is identical to the Gregorian calendar except it standardizes how weeks are defined. See https://en.wikipedia.org/wiki/ISO_8601#Week_dates for more information.

At least the year, month, and day properties must be present. If calendar is one that requires era (like the Japanese calendar), then the era property must also be present. Defaults for omitted fields are also determined by the calendar. For example, time fields default to zero for the iso8601 calendar, but hour defaults to 6 in the ethiopic calendar.

@ptomato ptomato added the documentation Additions to documentation label Sep 17, 2020
@ptomato ptomato added this to the Stable proposal milestone Sep 17, 2020
@ptomato ptomato added spec-text Specification text involved non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! and removed non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Sep 18, 2020
ptomato added a commit that referenced this issue Oct 7, 2020
@ptomato ptomato self-assigned this Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants