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

Add test for skipped initial days in Temporal.Calendar.prototype.yearMonthFromFields #4080

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

Conversation

anba
Copy link
Contributor

@anba anba commented May 14, 2024

The polyfill doesn't pass this test:

let pym = Temporal.PlainYearMonth.from({calendar: "japanese", era: "heisei", eraYear: 1, month: 1});
console.log(pym.getISOFields().isoDay);  // Prints 1
console.log(pym.era);  // Prints "showa"

Is this a bug in the polyfill or did I misinterpret this step from Temporal.Calendar.prototype.yearMonthFromFields?

7.c Let firstDayIndex be the 1-based index of the first day of the month described by fields (i.e., 1 unless the month's first day is skipped by this calendar.)

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

ptomato commented May 14, 2024

I think that might indeed be a misinterpretation of that step. That January spans two era/eraYear pairs, Showa 64 and Heisei 1. It's not two separate Januaries, so the PlainYearMonth correctly resolves to the era/eraYear pair in effect at the beginning of the month.

firstDayIndex is only intended to not be 1 if the calendar skips days at the beginning of the month outright, which none of the current ICU calendars do. (The only occurrence I'm aware of is in the switch from Julian to Gregorian calendar in some European regions, where they skipped days at the beginning of the month rather than in the middle.)

See also tc39/proposal-temporal#1315 and tc39/proposal-temporal#1300.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants