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

Reconsider year-week support in light of implementation feedback #2405

Closed
ptomato opened this issue Sep 15, 2022 · 5 comments · Fixed by #2425
Closed

Reconsider year-week support in light of implementation feedback #2405

ptomato opened this issue Sep 15, 2022 · 5 comments · Fixed by #2425
Assignees
Labels
needs plenary input Needs to be presented to the committee and feedback incorporated normative Would be a normative change to the proposal spec-text Specification text involved

Comments

@ptomato
Copy link
Collaborator

ptomato commented Sep 15, 2022

In the discussion about specifying ISOWeekOfYear (#2378) it came up that our limited support for year-week numbers can lead to bugs.

For example, Temporal.PlainDate.from('2022-01-01').weekOfYear is 52; January 1 2022 is day 6 of week 52 of the week-year 2021, and in ISO year-week notation (which we decided was out of scope for Temporal) would be notated 2021-W52-6, and referred to as being in "ISO week 52 of 2021". The current way to obtain the week-year is to rewind the date to the previous Monday and get the year of that date, which is not obvious.

Several options:

  1. Do nothing. Week numbers are often used in business contexts in Europe, but in the vast majority of use cases you don't need the week-year, you just need the week number, which Temporal correctly provides. Save full year-week-day support for a following proposal. Caution in the documentation against building ISO year-week notation strings using date.year and include an example of how to calculate the week-year in the cookbook.
  2. Decide that the likelihood of bugs arising from using date.year as the week-year is too high, and remove the weekOfYear API from Temporal, saving it for a following proposal. In the meantime, week numbers (and week-years in the few use cases where they are needed) can be calculated in userland. Include an example of how to do that in the cookbook.
  3. Decide that the likelihood of bugs arising from using date.year as the week-year is too high, and add a weekYear accessor property to PlainDate, PlainDateTime, and ZonedDateTime, or even replace the weekOfYear API with an API that returns both the week-year and the week number since even with weekYear people could still mistakenly use date.year as the week-year. Save full year-week-day support for a following proposal.
  4. Bring in full year-week-day support. I mention this for completeness, but I consider this thoroughly off the table.
@ptomato
Copy link
Collaborator Author

ptomato commented Sep 15, 2022

cc @sffc

@ljharb
Copy link
Member

ljharb commented Sep 15, 2022

I prefer, in order, 4, 3, and 1, and don't consider 2 worth doing.

@sffc
Copy link
Collaborator

sffc commented Sep 16, 2022

I prefer option 3. We should grow into option 4 but I'm not interested in holding up the proposal on this. We can do 3 now and 4 later.

In ICU4X, we return a single struct with the week number and the "RelativeYear" (which is either current, previous, or next).

https://unicode-org.github.io/icu4x-docs/doc/icu_calendar/week/struct.WeekOf.html

Note that for era and eraYear, we also combine those into a single struct in ICU4X.

https://unicode-org.github.io/icu4x-docs/doc/icu_calendar/types/struct.FormattableYear.html

My preference would be for weekOfYear to return a Tuple or Record with the info, but if that's not possible, then weekYear seems fine. Or, we just return an object, since I think it's safe to say that you seldom want one without the other.

@ptomato ptomato added the normative Would be a normative change to the proposal label Sep 28, 2022
@sffc
Copy link
Collaborator

sffc commented Oct 13, 2022

@pipobscure suggested weekOfYear and yearOfWeek.

I would just note that yearOfWeek in this case should be an arithmetic year, not an era year.

@ptomato
Copy link
Collaborator Author

ptomato commented Oct 14, 2022

In the Temporal champions meeting of 2022-10-13 we agreed that we want to present option 3 for consensus in November, with (as Shane said) the name yearOfWeek. For example, Temporal.PlainDate.from('2022-01-01').yearOfWeek would be 2021.

This would add a yearOfWeek getter on Temporal.PlainDate.prototype, Temporal.PlainDateTime.prototype, and Temporal.ZonedDateTime.prototype, and a new Temporal.Calendar.prototype.yearOfWeek() method. The algorithm for determining when year and yearOfWeek are different is basically already part of the ToISOWeekOfYear operation, so needs to be adapted slightly for the new API.

@ptomato ptomato added spec-text Specification text involved needs plenary input Needs to be presented to the committee and feedback incorporated and removed question meeting-agenda labels Oct 14, 2022
@ptomato ptomato self-assigned this Oct 20, 2022
ptomato added a commit to ptomato/test262 that referenced this issue Oct 21, 2022
To be presented for consensus in the November/December TC39 meeting. This
adds tests for a 'yearOfWeek' getter to PlainDate, PlainDateTime, and
ZonedDateTime, for use alongside 'weekOfYear', and tests for a
corresponding method to Calendar.

The tests are basically the existing tests of 'weekOfYear' adapted.

Temporal issue: tc39/proposal-temporal#2405
ptomato added a commit to ptomato/test262 that referenced this issue Oct 21, 2022
To be presented for consensus in the November/December TC39 meeting. This
adds tests for a 'yearOfWeek' getter to PlainDate, PlainDateTime, and
ZonedDateTime, for use alongside 'weekOfYear', and tests for a
corresponding method to Calendar.

The tests are basically the existing tests of 'weekOfYear' adapted.

Temporal issue: tc39/proposal-temporal#2405
ptomato added a commit that referenced this issue Oct 21, 2022
The rationale that we got from feedback during implementation is that it's
not obvious that the weekOfYear may belong to a different "week calendar
year" than the date's calendar year, which may lead to bugs like
unwittingly presenting the date 2022-01-01 as being in week 52 of 2022
(it is actually in week 52 of 2021.)

Additionally, the yearOfWeek isn't that straightforward to calculate
unless you are already calculating the week number.

Adds the following APIs:
  Temporal.PlainDate.yearOfWeek
  Temporal.PlainDateTime.yearOfWeek
  Temporal.ZonedDateTime.yearOfWeek
  Temporal.Calendar.yearOfWeek()
and updates the ToISOWeekOfYear and CalendarDateWeekOfYear abstract ops to
return a Record with [[Week]] and [[Year]] fields, instead of just a week
number.

Closes: #2405
ptomato added a commit that referenced this issue Oct 24, 2022
The rationale that we got from feedback during implementation is that it's
not obvious that the weekOfYear may belong to a different "week calendar
year" than the date's calendar year, which may lead to bugs like
unwittingly presenting the date 2022-01-01 as being in week 52 of 2022
(it is actually in week 52 of 2021.)

Additionally, the yearOfWeek isn't that straightforward to calculate
unless you are already calculating the week number.

Adds the following APIs:
  Temporal.PlainDate.yearOfWeek
  Temporal.PlainDateTime.yearOfWeek
  Temporal.ZonedDateTime.yearOfWeek
  Temporal.Calendar.yearOfWeek()
and updates the ToISOWeekOfYear and CalendarDateWeekOfYear abstract ops to
return a Record with [[Week]] and [[Year]] fields, instead of just a week
number.

Closes: #2405
ptomato added a commit that referenced this issue Nov 29, 2022
The rationale that we got from feedback during implementation is that it's
not obvious that the weekOfYear may belong to a different "week calendar
year" than the date's calendar year, which may lead to bugs like
unwittingly presenting the date 2022-01-01 as being in week 52 of 2022
(it is actually in week 52 of 2021.)

Additionally, the yearOfWeek isn't that straightforward to calculate
unless you are already calculating the week number.

Adds the following APIs:
  Temporal.PlainDate.yearOfWeek
  Temporal.PlainDateTime.yearOfWeek
  Temporal.ZonedDateTime.yearOfWeek
  Temporal.Calendar.yearOfWeek()
and updates the ToISOWeekOfYear and CalendarDateWeekOfYear abstract ops to
return a Record with [[Week]] and [[Year]] fields, instead of just a week
number.

Closes: #2405
ptomato added a commit to ptomato/test262 that referenced this issue Nov 30, 2022
To be presented for consensus in the November/December TC39 meeting. This
adds tests for a 'yearOfWeek' getter to PlainDate, PlainDateTime, and
ZonedDateTime, for use alongside 'weekOfYear', and tests for a
corresponding method to Calendar.

The tests are basically the existing tests of 'weekOfYear' adapted.

Temporal issue: tc39/proposal-temporal#2405
ptomato added a commit to tc39/test262 that referenced this issue Nov 30, 2022
To be presented for consensus in the November/December TC39 meeting. This
adds tests for a 'yearOfWeek' getter to PlainDate, PlainDateTime, and
ZonedDateTime, for use alongside 'weekOfYear', and tests for a
corresponding method to Calendar.

The tests are basically the existing tests of 'weekOfYear' adapted.

Temporal issue: tc39/proposal-temporal#2405
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs plenary input Needs to be presented to the committee and feedback incorporated normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants