-
Notifications
You must be signed in to change notification settings - Fork 508
DRAFT: Temporal: Add test for formatting Temporal objects with era formatter #4634
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
base: main
Are you sure you want to change the base?
Conversation
|
Awaiting tc39/proposal-temporal#3175 |
ptomato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
We should have similar tests for formatRange, formatToParts, formatRangeToParts, and the toLocaleString methods of PlainDate, PlainYearMonth, PlainMonthDay, PlainTime, PlainDateTime, Instant, and ZonedDateTime.
| assert(formatter.format(new Temporal.PlainYearMonth(2025, 11, "gregory")).startsWith("11"), "formatting a PlainYearMonth should work"); | ||
| assert(formatter.format(new Temporal.PlainMonthDay(11, 4, "gregory")).startsWith("11"), "formatting a PlainMonthDay should work"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using "gregory" may fail depending on the calendar that formatter gets from the host's locale. Probably the best way to work around this would be to hardcode the formatter to "en", and include locales: [en] in the frontmatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 8ae6c62
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/locales/locale apparently. Sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like the linter complains about including locales: [en] in the frontmatter? Or did I do that incorrectly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got it wrong, it's locale https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#locale
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in bd540ad
| assert(formatter.format(new Temporal.PlainDate(2025, 11, 4)).startsWith("11"), "formatting a PlainDate should work"); | ||
| assert(formatter.format(new Temporal.PlainYearMonth(2025, 11, "gregory")).startsWith("11"), "formatting a PlainYearMonth should work"); | ||
| assert(formatter.format(new Temporal.PlainMonthDay(11, 4, "gregory")).startsWith("11"), "formatting a PlainMonthDay should work"); | ||
| assert(formatter.format(new Temporal.PlainTime(14, 46)).startsWith("A"), "formatting a PlainTime should work"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised by this expectation... It does match what comes out of new Date().toLocaleTimeString('en', { era: 'narrow' }), but my reading of the spec (GetDateTimeFormat) is that era isn't copied to formatOptions when constructing [[TemporalPlainTimeFormat]].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed tc39/proposal-temporal#3175 (review) accordingly and updated this test.
| assert(formatter.format(new Temporal.PlainMonthDay(11, 4, "gregory")).startsWith("11"), "formatting a PlainMonthDay should work"); | ||
| assert(formatter.format(new Temporal.PlainTime(14, 46)).startsWith("A"), "formatting a PlainTime should work"); | ||
| assert(formatter.format(new Temporal.PlainDateTime(2025, 11, 4, 14, 46)).startsWith("11"), "formatting a PlainDateTime should work"); | ||
| assert(formatter.format(new Temporal.Instant(0n)).startsWith("12"), "formatting an Instant should work"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may fail depending on the host's time zone offset. Probably the best way to work around that is pass timeZone: 'UTC' in the formatter options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 28e653d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I gave you bad advice here, since the test is supposed to check what happens when you pass era with no other options.
Maybe a better approach would be to check that the output of formatter.format(new Temporal.Instant(0n)) is identical to that of new Date(0).toLocaleString(["en"], { era: "narrow" })?
See tc39/proposal-temporal#3049