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

Should we support alternate ways of telling time? #522

Closed
sffc opened this issue Apr 22, 2020 · 24 comments · Fixed by #818, #1062 or #1188
Closed

Should we support alternate ways of telling time? #522

sffc opened this issue Apr 22, 2020 · 24 comments · Fixed by #818, #1062 or #1188
Assignees
Labels
calendar Part of the effort for Temporal Calendar API documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@sffc
Copy link
Collaborator

sffc commented Apr 22, 2020

We allow custom calendars to use different fields for specifying a date (for example, the Japanese calendar can add era). However, according to #389, days are unconditionally defined as solar days on Earth, primarily so that we can have a 1-to-1 mapping with the ISO calendar system.

Should we allow calendars that use alternate ways of telling time within the solar day? For example, maybe a calendar uses something other than hours, minutes, and seconds for subdividing the day. I have heard of such calendars, but I don't know specific examples off the top of my head.

Additional Context: https://github.com/tc39/proposal-temporal/pull/520/files#r412515900

@ptomato ptomato added the calendar Part of the effort for Temporal Calendar API label Apr 22, 2020
@ryzokuken
Copy link
Member

Let's discuss this over the meeting, shall we?

@ptomato
Copy link
Collaborator

ptomato commented May 11, 2020

Meeting, May 7: Temporal won't support it at this time, but we'll take a future-proof approach by changing the names of the calendar methods to have date in them, so that we can later add time methods if a use case for this presents itself.

Action for @sffc, to make this change in the calendar explainer.

@jasonwilliams
Copy link
Member

What’s keeping this issue open?

@sffc
Copy link
Collaborator Author

sffc commented Jul 7, 2020

Action for @sffc, to make this change in the calendar explainer.

@ptomato ptomato assigned ptomato and unassigned sffc Aug 13, 2020
@ptomato
Copy link
Collaborator

ptomato commented Aug 13, 2020

Happy to take this one as well, I'm on a closing-issues spree

ptomato added a commit that referenced this issue Aug 13, 2020
Calendar.plus -> Calendar.datePlus
Calendar.minus -> Calendar.dateMinus
Calendar.difference -> Calendar.dateDifference

Rename some methods to have `date` in the name so that if there are use
cases for a time-based calendar then later timePlus, timeMinus, and
timeDifference can be added.

Closes: #522
ptomato added a commit that referenced this issue Aug 13, 2020
Calendar.plus -> Calendar.datePlus
Calendar.minus -> Calendar.dateMinus
Calendar.difference -> Calendar.dateDifference

Rename some methods to have `date` in the name so that if there are use
cases for a time-based calendar then later timePlus, timeMinus, and
timeDifference can be added.

Closes: #522
@pipobscure
Copy link
Collaborator

The decision here was based purely on not having use-cases. This needs revisiting.

@pipobscure pipobscure reopened this Sep 4, 2020
@ptomato
Copy link
Collaborator

ptomato commented Sep 5, 2020

Can you say more about the use cases so that we can discuss what changes will be necessary?

@sffc
Copy link
Collaborator Author

sffc commented Sep 5, 2020

I'm open to revisiting this discussion, but I would like to very strongly suggest that we do so with the following parameter: we retain "days" as a universal unit with the same length in all calendars. This assumption is what has enabled us to perform 1-to-1 conversion between calendar systems and ISO-8601, which is the basis of the Temporal data model. In other words, it's totally okay with me if the calendar redefines how the clock works, as long as the clock is periodic each ISO day solar day.

Ms2ger pushed a commit that referenced this issue Nov 5, 2020
This is needed in order for ZonedDateTime.with() to correctly throw when
given a Time object.

See: #522
See: #569
ptomato added a commit that referenced this issue Nov 11, 2020
This was a straggler from the time calendar support (#522)
Ms2ger pushed a commit that referenced this issue Nov 11, 2020
This was a straggler from the time calendar support (#522)
@ptomato
Copy link
Collaborator

ptomato commented Nov 12, 2020

Meeting, Nov. 12: Based on reexamining of the use cases (see #1087 and https://unicode-org.atlassian.net/browse/CLDR-9716) we are not certain that we've properly understood how time calendars should be implemented and have decided to leave them out of the current proposal.

Things to consider:

  • Nearly everyone agrees that time calendars need more research. It could be that they need to be combined with date calendars, or it could be that they need to be orthogonal, or it could be that they will never be added.
  • Leaving out the calendar property of PlainTime and adding one later could break the web.
  • @pipobscure feels strongly that PlainTime should keep the calendar property and have it be fixed to one value, and a future revision could either allow other calendars or remove the property, both of which would be web-compatible. Adding other calendars could break existing code, but only if the developer opted in to receiving PlainTimes with non-ISO calendars.
  • @sffc feels strongly that PlainTime should remove the calendar property because otherwise we don't provide an ergonomic solution to combining dates with times, and solutions harmful for l10n such as with({ ...time.getFields(), calendar: undefined }) will proliferate on Stack Overflow.

This is a solution that we believe addresses everyone's concerns:

  • Add PlainDateTime.withPlainDate(), PlainDateTime.withPlainTime(), ZonedDateTime.withPlainDate(), and ZonedDateTime.withPlainTime() to address the use case of combining dates and times without spreading. These methods use the 'consolidate' behaviour for calendars (throw if non-ISO and different).
  • Remove Calendar.timeFromFields(), Calendar.timeAdd(), Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
  • Remove PlainTime.withCalendar().
  • Remove the calendar argument from PlainTime constructor.
  • Throw if there's a non-ISO calendar passed to PlainTime.from().
  • The value of PlainTime.calendar is always the ISO calendar.
  • Leave the names of the slots, the properties returned from PlainTime.getFields() and PlainTime.getISOFields().

@justingrant
Copy link
Collaborator

One note to add to above now that #1087 is closed:

  • plainTime.toPlainDateTime(plainDate), plainDate.toPlainDateTime(plainTime), and their ZDT counterparts should all use the 'consolidate' behaviour for calendars (throw if non-ISO and different). This means that withTime and withDate act the same as toPlainDateTime and toZonedDateTime.

@ptomato
Copy link
Collaborator

ptomato commented Nov 13, 2020

One more amendment, I'm assuming we will remove now.plainTime(), and for maximum forward compatibility we should not rename plainTimeISO to plainTime?

@justingrant
Copy link
Collaborator

One more amendment, I'm assuming we will remove now.plainTime(), and for maximum forward compatibility we should not rename plainTimeISO to plainTime?

There is no plainTime on now, at least according to the docs: https://tc39.es/proposal-temporal/docs/now.html#plainTimeISO

It's a bummer that we're committing to a strange name for the "get the current local time" API in return for a potential future feature that we know will be rarely used even in the future. But I guess it's at least consistent with the pattern used elsewhere where any time developers want the "normal" result they'll need to stick an "ISO" at the end of the method name. :-(

@ptomato
Copy link
Collaborator

ptomato commented Nov 13, 2020

I think that was an oversight in the docs, it was present in the polyfill.

@sffc
Copy link
Collaborator Author

sffc commented Nov 13, 2020

It seems kind-of silly that we have a calendar slot on PlainTime that doesn't do anything.

I'm okay with Temporal.now.plainTimeISO(). Both "plain" and "ISO" are superfluous.

ptomato added a commit that referenced this issue Nov 14, 2020
- Remove Calendar.timeFromFields(), Calendar.timeAdd(),
  Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
- Remove PlainTime.withCalendar().
- Remove now.plainTime().
- Remove the calendar argument from PlainTime constructor.
- Remove the calendarName option from PlainTime.toString().
- Throw if there's a non-ISO calendar passed to PlainTime.from().
- The value of PlainTime.calendar is always the ISO calendar.
- Leave the names of the slots, the properties returned from
  PlainTime.getFields() and PlainTime.getISOFields().

See: #522
@ryzokuken
Copy link
Member

One more amendment, I'm assuming we will remove now.plainTime(), and for maximum forward compatibility we should not rename plainTimeISO to plainTime?

SGTM

ptomato added a commit that referenced this issue Nov 16, 2020
- Remove Calendar.timeFromFields(), Calendar.timeAdd(),
  Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
- Remove PlainTime.withCalendar().
- Remove now.plainTime().
- Remove the calendar argument from PlainTime constructor.
- Remove the calendarName option from PlainTime.toString().
- Throw if there's a non-ISO calendar passed to PlainTime.from().
- The value of PlainTime.calendar is always the ISO calendar.
- Leave the names of the slots, the properties returned from
  PlainTime.getFields() and PlainTime.getISOFields().

See: #522
ptomato added a commit that referenced this issue Nov 16, 2020
- Remove Calendar.timeFromFields(), Calendar.timeAdd(),
  Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
- Remove PlainTime.withCalendar().
- Remove now.plainTime().
- Remove the calendar argument from PlainTime constructor.
- Remove the calendarName option from PlainTime.toString().
- Throw if there's a non-ISO calendar passed to PlainTime.from().
- The value of PlainTime.calendar is always the ISO calendar.
- Leave the names of the slots, the properties returned from
  PlainTime.getFields() and PlainTime.getISOFields().

See: #522
ptomato added a commit that referenced this issue Nov 16, 2020
- Remove Calendar.timeFromFields(), Calendar.timeAdd(),
  Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
- Remove PlainTime.withCalendar().
- Remove now.plainTime().
- Remove the calendar argument from PlainTime constructor.
- Remove the calendarName option from PlainTime.toString().
- Throw if there's a non-ISO calendar passed to PlainTime.from().
- The value of PlainTime.calendar is always the ISO calendar.
- Leave the names of the slots, the properties returned from
  PlainTime.getFields() and PlainTime.getISOFields().

See: #522
ptomato added a commit that referenced this issue Nov 16, 2020
- Remove Calendar.timeFromFields(), Calendar.timeAdd(),
  Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
- Remove PlainTime.withCalendar().
- Remove now.plainTime().
- Remove the calendar argument from PlainTime constructor.
- Remove the calendarName option from PlainTime.toString().
- Throw if there's a non-ISO calendar passed to PlainTime.from().
- The value of PlainTime.calendar is always the ISO calendar.
- Leave the names of the slots, the properties returned from
  PlainTime.getFields() and PlainTime.getISOFields().

See: #522
ptomato added a commit that referenced this issue Nov 16, 2020
- Remove Calendar.timeFromFields(), Calendar.timeAdd(),
  Calendar.timeUntil(), Calendar.hour(), ..., Calendar.nanosecond().
- Remove PlainTime.withCalendar().
- Remove now.plainTime().
- Remove the calendar argument from PlainTime constructor.
- Remove the calendarName option from PlainTime.toString().
- Throw if there's a non-ISO calendar passed to PlainTime.from().
- The value of PlainTime.calendar is always the ISO calendar.
- Leave the names of the slots, the properties returned from
  PlainTime.getFields() and PlainTime.getISOFields().

See: #522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calendar Part of the effort for Temporal Calendar API documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
7 participants