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

Adjust Calendar/prototype/yearMonthFromFields/reference-day.js for alternative implementations #4079

Merged
merged 2 commits into from
May 29, 2024

Conversation

anba
Copy link
Contributor

@anba anba commented May 14, 2024

  • 74dec5b splits "test/intl402/Temporal/Calendar/prototype/yearMonthFromFields/reference-day.js" into three files, so implementation don't have to skip the whole test when they don't implement a specific calendar.
  • 4785479 adjusts the Chinese calendar test to allow ICU4X-based implementations to pass the test.
    • The Chinese calendar doesn't have a well-defined "epoch year". ICU4X uses 2637 BCE as the start of the "epoch year". See also https://docs.rs/icu/latest/icu/calendar/chinese/struct.Chinese.html#year-and-era-codes.
    • ICU4C and ICU4X don't always agree on which years are leap years. Change the related year of "M01L" from 2148 to 1651, and for "M11L" from 2033 to 1517 to use years which work in both ICU4C and ICU4X. ICU4C and ICU4X don't agree on any year which uses "M12L", so we have to delete this entry.

@anba anba requested a review from a team as a code owner May 14, 2024 13:38
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 100% on the splitting.

Re. M12L, see https://unicode-org.atlassian.net/browse/ICU-22230. It's unfortunate that we have to do this, but as you say, we would need to understand why implementations differ on this point and find an authoritative source.

cc @FrankYFTang @justingrant @sffc who may be interested.

@anba
Copy link
Contributor Author

anba commented May 15, 2024

Re. M12L, see https://unicode-org.atlassian.net/browse/ICU-22230. It's unfortunate that we have to do this, but as you say, we would need to understand why implementations differ on this point and find an authoritative source.

Comparing ICU4X (through in-progress Temporal patches for SM) and ICU4C (through the spec polyfill), there appear to be four cases where ICU4X and ICU4C don't agree on leap months in the years 1900...2100. (Years too much in the past/future are probably not worth worrying about.)

Year ICU4X ICU4C
1917 M02L M03L
1922 M05L M06L
1987 M06L M07L
2033 M07L M11L

https://www.hko.gov.hk/en/gts/time/conversion.htm agrees with the ICU4X results for 1917, 1922, and 1987, but for 2033 it agrees with ICU4C. [1]

The conversion tables are provides as pdf-files (e.g. https://www.hko.gov.hk/tc/gts/time/calendar/pdf/files/1987.pdf) and text-files (e.g. https://www.hko.gov.hk/en/gts/time/calendar/text/files/T1987e.txt).

(I didn't find conversion tables issued by the Purple Mountain Observatory, so hopefully the data from the Hong Kong Observatory matches what's issued by PMO.)

[1] https://ytliu0.github.io/ChineseCalendar/index.html?y=2033 also computes M11L for 2033.

@anba
Copy link
Contributor Author

anba commented May 15, 2024

https://www.xirugu.com/CHI500/Dates_Time/Chinesecalender.pdf also mentions that 2033 is a special-case, see section §4.4 "Why is 2033 an exceptional year?". That might explain the ICU4X results.

@anba
Copy link
Contributor Author

anba commented May 28, 2024

Can this PR get merged, so I can prepare some follow-up changes to update tests to allow alternative era strings? Thanks!

@justingrant
Copy link
Contributor

Note that @ptomato is out on vacation this week.

@anba
Copy link
Contributor Author

anba commented May 28, 2024

Note that @ptomato is out on vacation this week.

The PR is already approved, it just needs someone with commit access to merge the changes.

@justingrant
Copy link
Contributor

@Ms2ger would you be able to help merge this?

@anba, looks like a rebase is needed.

@Ms2ger Ms2ger enabled auto-merge (rebase) May 29, 2024 08:44
@Ms2ger Ms2ger merged commit 57b9f15 into tc39:main May 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants