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

Further clarifications of calendar-dependent parts of yearMonthFromFields and monthDayFromFields #2461

Closed
ptomato opened this issue Jan 6, 2023 · 7 comments · Fixed by #2475
Assignees
Labels
calendar Part of the effort for Temporal Calendar API normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@ptomato
Copy link
Collaborator

ptomato commented Jan 6, 2023

Spotted by @justingrant in #1502.

Temporal.Calendar.prototype.yearMonthFromFields and Temporal.Calendar.prototype.monthDayFromFields both go through CalendarDateToISO in the non-ISO case. This operation is implementation-defined, but doesn't say what to do if there isn't enough information on fields to produce a full ISO date, which would be the case in yearMonthFromFields and monthDayFromFields.

For yearMonthFromFields we can simply add a step "Perform ! CreateDataPropertyOrThrow(fields, "day", 1F)" but for monthDayFromFields we need to somehow describe the requirement to pick a reference year in the implementation-defined part. I'd suggest that we use a separate implementation-defined operation, CalendarMonthDayToISOReferenceDate.

@ptomato ptomato added spec-text Specification text involved calendar Part of the effort for Temporal Calendar API normative Would be a normative change to the proposal labels Jan 6, 2023
@ptomato ptomato added this to the Stage "3.5" milestone Jan 6, 2023
@justingrant
Copy link
Collaborator

To encourage consistency across implementations, maybe use language something like below for determining the reference year?

Implementations must deterministically choose the same referenceISOYear for any particular month and day pair in each calendar. To calculate the referenceISOYear, implementations must pick the latest ISO year that is equal to or less than 1972. If no ISO years before 1972 contain that month/day pair, then use the earliest ISO year that is larger than 1972. The reference year will almost always be 1972, with rare exceptions for some calendars where some dates (e.g. leap days or days in leap months) didn't occur during ISO year 1972.

@ptomato ptomato self-assigned this Jan 13, 2023
ptomato added a commit that referenced this issue Jan 14, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Jan 14, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Jan 16, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Jan 16, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Jan 16, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Jan 17, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Jan 18, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Jan 18, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Jan 20, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
@Akxe
Copy link

Akxe commented Feb 4, 2023

May I ask; Why does Temporal.PlainYearMonth.until(Temporal.PlainYearMonth, unit), require the relativeTo to work?

Is it a bug? Should it be able to diff two PlainYearMonth when unit is bigger or equate to month?

@ptomato
Copy link
Collaborator Author

ptomato commented Feb 6, 2023

May I ask; Why does Temporal.PlainYearMonth.until(Temporal.PlainYearMonth, unit), require the relativeTo to work?

Is it a bug? Should it be able to diff two PlainYearMonth when unit is bigger or equate to month?

Thanks for the report! As far as I know, no relativeTo is required in that call. Here's what I tested:

Temporal.PlainYearMonth.from('2021-06').until(Temporal.PlainYearMonth.from('2023-02'), {largestUnit: 'month', smallestUnit: 'month'})
  // => duration of P20M (20 months)

Please open a separate issue with a code example of what is not working, as I think it's probably not related to this.

@Akxe
Copy link

Akxe commented Feb 7, 2023

My bad; it was the next part... the total on a duration

Temporal.PlainYearMonth.from('2021-06')
  .until(Temporal.PlainYearMonth.from('2023-02'))
  .total({ unit: 'month' })

The snippet above does throw in the docs playground.

Passing relativeTo: Temporal.Now.plainDateTimeISO(), does fix the issue, but I don't think it should require it in the first place...

@justingrant
Copy link
Collaborator

justingrant commented Feb 7, 2023

The problem you're running into is that, by the time total is called, the code doesn't know that only months are possible. Your sample above is equivalent to this: Temporal.Duration.from('P1Y8M').total({ unit: 'month' }).

While it's true that the duration P1Y8M is evenly divisible into months, if you had a different duration like P8M30DT20H then you'd need a relativeTo in order to accurately calculate the fractional part of the total.

One of our principles in designing Temporal's APIs is that determining which options are valid vs. invalid for a particular operation is performed only on the shape (meaning what properties & types) of the input(s), rather than the value of those inputs. This makes it easier to catch bugs while code is in development because the shape of inputs is usually known at development time, while their values are usually not determined until runtime. The downside of this design principle is that sometimes you need to use more verbose code in order to ensure that options will be accepted regardless of the values of the inputs.

Anyway, if you want to avoid specifying a relativeTo option, then you can get the same result by simply limiting the units returned by until. Like this:

Temporal.PlainYearMonth.from('2021-06')
  .until(Temporal.PlainYearMonth.from('2023-02'), { smallestUnit: 'month', largestUnit: 'month' }).months;

@Akxe
Copy link

Akxe commented Feb 7, 2023

The problem you're running into is that, by the time total is called, the code doesn't know that only months are possible. Your sample above is equivalent to this: Temporal.Duration.from('P1Y8M').total({ unit: 'month' }).

While it's true that the duration P1Y8M is evenly divisible into months, if you had a different duration like P8M30DT20H then you'd need a relativeTo in order to accurately calculate the fractional part of the total.

It took me a while to wrap my head around it, but I get it now...

One of our principles in designing Temporal's APIs is that determining which options are valid vs. invalid for a particular operation is performed only on the shape (meaning what properties & types) of the input(s), rather than the value of those inputs. This makes it easier to catch bugs while code is in development because the shape of inputs is usually known at development time, while their values are usually not determined until runtime. The downside of this design principle is that sometimes you need to use more verbose code in order to ensure that options will be accepted regardless of the values of the inputs.

Does this mean that the values (rather than shape) will be checked once Temporal is implemented into browsers?

@justingrant
Copy link
Collaborator

Does this mean that the values (rather than shape) will be checked once Temporal is implemented into browsers?

Nope, the behavior of native implementations should match the current polyfills because both of them are built from the same underlying spec.

To be clear, all polyfills and native implementations do validate values of inputs. If an invalid value is provided, then an exception will be thrown. But determining which options are required vs. optional usually relies only on the shape of the input(s).

ptomato added a commit that referenced this issue Feb 8, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Feb 8, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Feb 15, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Feb 15, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Feb 18, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Feb 18, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
ptomato added a commit that referenced this issue Feb 18, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: #2461
ptomato added a commit that referenced this issue Feb 18, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: #2461
Aditi-1400 pushed a commit to Aditi-1400/proposal-temporal that referenced this issue Mar 7, 2023
In order for CalendarDateToISO to work, we have to specify in _fields_
that we want the first of the month.

See: tc39#2461
Aditi-1400 pushed a commit to Aditi-1400/proposal-temporal that referenced this issue Mar 7, 2023
This was missing from the non-ISO part of monthDayFromFields(). Calendars
must choose a reference ISO year when creating a PlainMonthDay. This adds
language specifying how the reference year is deterministically chosen, in
a separate calendar operation from CalendarDateToISO.

See: tc39#2461
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 normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
3 participants