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

Need to be more specific about "according to ISO-8601" in ToISOWeekOfYear #1641

Closed
FrankYFTang opened this issue Jul 14, 2021 · 25 comments
Closed
Labels
editorial spec-text Specification text involved

Comments

@FrankYFTang
Copy link
Contributor

FrankYFTang commented Jul 14, 2021

Three of the AOs in Temporal has the language "according to ISO-8601"
https://tc39.es/proposal-temporal/#sec-temporal-toisodayofweek
https://tc39.es/proposal-temporal/#sec-temporal-toisodayofyear
https://tc39.es/proposal-temporal/#sec-temporal-toisoweekofyear

12.1.33 ToISODayOfWeek ( year, month, day )
1. Assert: year is an integer.
2. Assert: month is an integer.
3. Assert: day is an integer.
4. Let date be the date given by year, month, and day.
5. Return date's day of the week according to ISO-8601.
NOTE
Monday is 1 and Sunday is 7.
12.1.34 ToISODayOfYear ( year, month, day )
1. Assert: year is an integer.
2. Assert: month is an integer.
3. Assert: day is an integer.
4. Let date be the date given by year, month, and day.
5. Return date's ordinal date in the year according to ISO-8601.
12.1.35 ToISOWeekOfYear ( year, month, day )
1. Assert: year is an integer.
2. Assert: month is an integer.
3. Assert: day is an integer.
4. Let date be the date given by year, month, and day.
5. Return date's week number according to ISO-8601.

Please add link to ISO-8601 or add text to exaplain how that work.

and which revision of ISO-8601 are we talking about?
My understanding is ISO standard are not freely available but need to be purchase. IF we do not explain how does it work in these AO then it is hard to implement it or write test against it. RFC or other standard is much easier to reference to. By looking at these three AOs above, I have no clue how to implement them.

@justingrant @ptomato @Ms2ger @ryzokuken

@ryzokuken
Copy link
Member

The spec includes a grammar that should be followed, which is basically a profile of the ISO-8601 grammar in ecmarkup.

@FrankYFTang
Copy link
Contributor Author

The spec includes a grammar that should be followed, which is basically a profile of the ISO-8601 grammar in ecmarkup.

sure, that work for all the Parse* AOs and great, but how would the ISO-8601 grammar to tell me how to
"Return date's day of the week according to ISO-8601." with an integer of year, month and day?
or
"Return date's ordinal date in the year according to ISO-8601." with an integer of year, month and day?
or
"Return date's week number according to ISO-8601." with an integer of year, month and day?

What I am asking is for Temporal to include similar level of text as ISO-8601 grammar from ISO8601 to answer how to implement these 3 AOs
ToISODayOfWeek / ToISODayOfYear / ToISOWeekOfYear

I do not believe the grammar can be used to answer that. IF it could, please show me one as example. Thanks

@FrankYFTang
Copy link
Contributor Author

For example, from the spec itself, I don't think we can easily figure out the answer for the following and justify the value for the expected test, right?

let cal = new Temporal.Calendar("iso8601");
assertEquals(53, cal.weekOfYear(new Temporal.PlainDate(1977, 01, 01)));
assertEquals(53, cal.weekOfYear(new Temporal.PlainDate(1977, 01, 02)));

assertEquals(52, cal.weekOfYear(new Temporal.PlainDate(1977, 12, 31)));
assertEquals(52, cal.weekOfYear(new Temporal.PlainDate(1978, 01, 01)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1978, 01, 02)));

assertEquals(52, cal.weekOfYear(new Temporal.PlainDate(1978, 12, 31)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1979, 01, 01)));

assertEquals(52, cal.weekOfYear(new Temporal.PlainDate(1979, 12, 30)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1979, 12, 31)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1980, 01, 01)));

assertEquals(52, cal.weekOfYear(new Temporal.PlainDate(1980, 12, 28)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1980, 12, 29)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1980, 12, 30)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1980, 12, 31)));
assertEquals(1, cal.weekOfYear(new Temporal.PlainDate(1981, 01, 01)));

assertEquals(53, cal.weekOfYear(new Temporal.PlainDate(1981, 12, 31)));
assertEquals(53, cal.weekOfYear(new Temporal.PlainDate(1982, 01, 01)));
assertEquals(53, cal.weekOfYear(new Temporal.PlainDate(1982, 01, 02)));
assertEquals(53, cal.weekOfYear(new Temporal.PlainDate(1982, 01, 03)));

right?

@FrankYFTang
Copy link
Contributor Author

for example, with the current spec text, it is very hard to review the above two PRs for test262 and reach conclusion that the tests were written correctly.

@ptomato ptomato added the spec-text Specification text involved label Sep 2, 2021
@ptomato
Copy link
Collaborator

ptomato commented Sep 2, 2021

Meeting, Sept. 2: We determined that adding a reference to the appropriate section in ISO 8601 should be sufficient.

@FrankYFTang
Copy link
Contributor Author

Please be specific which PART of ISO 8601 and which section in that standard. I have hard time to locate them.
Notice ISO 8601 Part 1 is
"ISO 8601-1:2019 Date and time — Representations for information interchange — Part 1: Basic rules"
https://www.iso.org/standard/70907.html

and there are also
"ISO 8601-1:2019/DAMD 1
Date and time — Representations for information interchange — Part 1: Basic rules — Amendment 1"
https://www.iso.org/standard/81801.html

and there are Part 2 too
"ISO 8601-2:2019 Date and time — Representations for information interchange — Part 2: Extensions"
https://www.iso.org/standard/70908.html

and there are also previous version
"ISO 8601:2004 Data elements and interchange formats — Information interchange — Representation of dates and times"
https://www.iso.org/standard/40874.html

and
"ISO 8601:2000 Data elements and interchange formats — Information interchange — Representation of dates and times"
https://www.iso.org/standard/26780.html

and
"ISO 8601:1988 Data elements and interchange formats — Information interchange — Representation of dates and times"
https://www.iso.org/standard/15903.html

I assume we are referring to Part 1 of the 2019 version? Is that correc?
https://www.iso.org/obp/ui/#iso:std:iso:8601:-1:ed-1:v1:en

@FrankYFTang
Copy link
Contributor Author

I really think at minimum you need to specify
ToISOWeekOfYear
https://tc39.es/proposal-temporal/#sec-temporal-toisoweekofyear
currently the text is

1. [Assert](https://tc39.es/ecma262/#assert): year is an [integer](https://tc39.es/ecma262/#integer).
2. [Assert](https://tc39.es/ecma262/#assert): month is an [integer](https://tc39.es/ecma262/#integer).
3. [Assert](https://tc39.es/ecma262/#assert): day is an [integer](https://tc39.es/ecma262/#integer).
4. Let date be the date given by year, month, and day.
5. Return date's week number according to ISO-8601 as an [integer](https://tc39.es/ecma262/#integer).

@FrankYFTang FrankYFTang changed the title Need more info about "according to ISO-8601" Need to be more specific about "according to ISO-8601" in ToISOWeekOfYear Mar 31, 2022
@FrankYFTang
Copy link
Contributor Author

Notice my check in to v8 to implement
12.4.15 Temporal.Calendar.prototype.weekOfYear ( temporalDateLike )
is now blocked by this in
https://chromium-review.googlesource.com/c/v8/v8/+/3531567

Reviewer does not feel comfortable to approve my check in for an under specified specification text.

@sffc @ryzokuken

@FrankYFTang
Copy link
Contributor Author

FrankYFTang commented Apr 1, 2022

I read ISO 8601-1 , it does not specified how to convert from "year, month, and day" to week. It only define WHAT is "date's week number" without defining how to calculate it from year, month and day. We need the spec text to define HOW it is calculated here.

ptomato added a commit that referenced this issue May 6, 2022
ECMA-262 already has a DayWithinYear equation. We had used some handwavy
text in ToISODayOfYear about "the ordinal date in the year according to
ISO-8601" but this change uses the logic already present in the spec.

See: #1641
ptomato added a commit that referenced this issue May 6, 2022
ECMA-262 already has a WeekDay equation. We had used some handwavy text in
ToISODayOfWeek about "the day of week according to ISO-8601" but this
change uses the logic already present in the spec.

See: #1641
ptomato added a commit that referenced this issue May 6, 2022
ECMA-262 already has a DayWithinYear equation. We had used some handwavy
text in ToISODayOfYear about "the ordinal date in the year according to
ISO-8601" but this change uses the logic already present in the spec.

See: #1641
ptomato added a commit that referenced this issue May 6, 2022
ECMA-262 already has a WeekDay equation. We had used some handwavy text in
ToISODayOfWeek about "the day of week according to ISO-8601" but this
change uses the logic already present in the spec.

See: #1641
@ptomato
Copy link
Collaborator

ptomato commented May 6, 2022

@FrankYFTang #2178 should improve the situation for dayOfWeek and dayOfYear. I am still looking for a good reference for weekOfYear.

Ms2ger pushed a commit that referenced this issue May 6, 2022
ECMA-262 already has a DayWithinYear equation. We had used some handwavy
text in ToISODayOfYear about "the ordinal date in the year according to
ISO-8601" but this change uses the logic already present in the spec.

See: #1641
Ms2ger pushed a commit that referenced this issue May 6, 2022
ECMA-262 already has a WeekDay equation. We had used some handwavy text in
ToISODayOfWeek about "the day of week according to ISO-8601" but this
change uses the logic already present in the spec.

See: #1641
@FrankYFTang
Copy link
Contributor Author

Just FYI, I am very closed to land all of my (non 402 version) of Temporal.Calendar code (the last CL got checked in but reverted last night for a simple issue, I will reland it today) EXCEPT this issue inside ToISOWeekOfYear blacking the review of Temporal.Calendar.prototype.weekOfYear. No need to rush since I still have a lot of other task need to complete the landing of other classes anyway. I will work on the (only support UTC version) of Temporal.TimeZone next. For anyone who like to see the progress, I post the tracking of the landing and all related CLs on https://docs.google.com/document/d/1UmZYIbe1Za4FtT8JvxLHx2yFtHqyuJhrjeo99D_IVk8

@FrankYFTang
Copy link
Contributor Author

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2022

Note to myself, here is what HTML says about week numbers: https://html.spec.whatwg.org/#weeks

@FrankYFTang
Copy link
Contributor Author

@pdunkel - are you going to work on better specifying https://tc39.es/proposal-temporal/#sec-temporal-toisoweekofyear ?

@ptomato
Copy link
Collaborator

ptomato commented Jun 21, 2022

I'm intending to do it, but there are a lot of PRs that I want to review before the July agenda deadline first. If someone else wants to work on it, that would be fine with me.

@FrankYFTang
Copy link
Contributor Author

I landed most of the non-intl implementation to v8 now except since/until of PlainDateTime and ZoneDateTime (see https://chromium-review.googlesource.com/c/v8/v8/+/3750098 ) and the implementation of this (see https://chromium-review.googlesource.com/c/v8/v8/+/3531567 ) Please raise the priority to be higher. If you think the algorithm I used in https://chromium-review.googlesource.com/c/v8/v8/+/3531567/4/src/objects/js-temporal-objects.cc is reasonable and conforming to ISO8601, we may convert that to spec text. Also how does the polyfill implement this? Can we convert that to the spec text?

@FrankYFTang
Copy link
Contributor Author

Polyfill currently has the implementation of ToISOWeekOfYear in
https://github.com/tc39/proposal-temporal/blob/main/polyfill/lib/ecmascript.mjs#L2387
as

  WeekOfYear: (year, month, day) => {
    let doy = ES.DayOfYear(year, month, day);
    let dow = ES.DayOfWeek(year, month, day) || 7;
    let doj = ES.DayOfWeek(year, 1, 1);

    const week = MathFloor((doy - dow + 10) / 7);

    if (week < 1) {
      if (doj === 5 || (doj === 6 && ES.LeapYear(year - 1))) {
        return 53;
      } else {
        return 52;
      }
    }
    if (week === 53) {
      if ((ES.LeapYear(year) ? 366 : 365) - doy < 4 - dow) {
        return 1;
      }
    }

    return week;
  },

Can we just turn this into algorithm to for the spec text?

@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

I don't know the provenance of this code, and can't guarantee that it's bug-free, so I'm not sure it's the best basis for a spec algorithm. If possible, I would like to avoid writing our own algorithm in the spec, and refer to one from a reliable source instead.

Here's an overview of the research I've done so far, if you'd like to work on this. I think the most comprehensive explanation is given in https://webspace.science.uu.nl/~gent0113/calendar/isocalendar.htm which is basically an algorithm with a 400-year-long lookup table to verify it with. This seems like the most promising source to refer to and/or build a spec algorithm from.

I also found https://myweb.ecu.edu/mccartyr/ISOwdALG.txt and https://howardhinnant.github.io/date_algorithms.html and then this Perl module https://metacpan.org/pod/Date::WeekOfYear.

Ideally a book or a published peer-reviewed paper would be best, but it's been so long since I searched academia that I'm not sure I even remember where to start. I wonder if it would be acceptable to the editors to refer to the The Mathematics of ISO 8601 page, or maybe one of the sources they refer to.

@sffc
Copy link
Collaborator

sffc commented Aug 16, 2022

Does the spec contain the logic required to convert an ISO DateTime to a timestamp (seconds since epoch)?

If yes, then we've already opened the can of works of putting parts of the ISO 8601 arithmetic logic into ECMA-262, and I think we should likely add other pieces of it that Temporal requires, such as the week-of-year math.

@ptomato
Copy link
Collaborator

ptomato commented Aug 16, 2022

We have MakeDay which does not contain a description of the logic for converting an ISO calendar date to a timestamp, but on the other hand we have DaysInYear which does contain a full description of how to tell whether an ISO year is a leap year.

@pipobscure
Copy link
Collaborator

I cobbled this together from the ISO spec. So with minor adjustments it’s a good base.

Adjustment ES.DayOfWeek here conforms to the Date.prototype.getDay schema of having Sunday be 0 rather than the ISO schema where Sunday is 7. So for an algortithm spec that needs smoothing out.

The value of week can roughly be described as:

  1. how many Thursdays have happened in that year?
  2. if no Thursdays have happened the it’s the last week of the previous year

@ljharb
Copy link
Member

ljharb commented Aug 16, 2022

would an alternative for the day stuff be "get day, plus 1, mod 7"?

@pipobscure
Copy link
Collaborator

would an alternative for the day stuff be "get day, plus 1, mod 7"?

No, because

  • getDay(Monday) = 1
  • 1+1 mod 7 = 2

where the correct value for Monday is 1

unless I misunderstood what you mean

@ptomato
Copy link
Collaborator

ptomato commented Aug 23, 2022

Closed by #2378.

@ptomato ptomato closed this as completed Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

6 participants