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

Allow alternative era for Chinese calendar #4309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anba
Copy link
Contributor

@anba anba commented Nov 5, 2024

Year 1 in the Chinese calendar corresponds to 2637 BCE in ICU4X.

See https://docs.rs/icu/latest/icu/calendar/chinese/struct.Chinese.html#year-and-era-codes.

@anba anba requested a review from a team as a code owner November 5, 2024 09:11
@ptomato
Copy link
Contributor

ptomato commented Nov 7, 2024

Similar to #4101, I'm hesitant to modify the tests to allow both results here. Either the ICU4C result is correct, or the ICU4X result is. That's something that ICU4X needs to sort out.

@anba
Copy link
Contributor Author

anba commented Nov 7, 2024

This is different from #4101, because the different epoch year isn't a ICU4C vs. ICU4X difference, but a design decision from the Temporal polyfill. The Temporal polyfill formats dates using Intl.DateTimeFormat, which outputs in many cases the related Gregorian year, cf. new Intl.DateTimeFormat("en-u-ca-chinese").formatToParts(new Date).

Reading https://github.com/unicode-org/icu/blob/0357501948d2f0ab43c891f446e68f19b07b442d/icu4c/source/i18n/chnsecal.h#L52-L60, it seems like ICU4C and ICU4X actually agree on 2637 BCE as the initial epoch year.

And a similar modification was already allowed in #4079.

@ptomato
Copy link
Contributor

ptomato commented Nov 7, 2024

OK, thanks, got it. If ICU4C and ICU4X both agree on the result, then my comment doesn't apply.

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.

2 participants