fix(intl): #5840 — Intl.DateTimeFormat dayPeriod-only formatting (6 test262 cases) - #5862
Conversation
…est262 cases)
new Intl.DateTimeFormat('en', {dayPeriod: 'long'}) (no other date/time
component options) silently fell back to the default M/D/YYYY date string in
both format and formatToParts: dtf_primary_mask never counted dayPeriod
toward its time-dimension bit, and format_components/build_parts_from_components
had no dayPeriod rendering path at all.
- day_period_string(hour, style): en CLDR day-period boundaries at hour
granularity, verified empirically against node.
- dtf_primary_mask sets BIT_TIME for dayPeriod so a dayPeriod-only DTF isn't
misclassified as having no primary fields.
- format_components/build_parts_from_components thread a day_period_opt
through: dayPeriod-only renders just the period text; combined with a
numeric hour it renders "<hour> <period>", replacing the AM/PM suffix.
Unchanged when day_period_opt is None.
Fixes the 6 dayPeriod-*-en.js cases from the #5840 worklist with zero
regressions (verified via scripts/test262_subset.py under TZ=UTC).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds ChangesdayPeriod formatting support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DateCollator
participant DayPeriodString
Caller->>DateCollator: formatToParts(options with KEY_DAY_PERIOD)
DateCollator->>DateCollator: extract day_period_opt
DateCollator->>DateCollator: build_parts_from_components(day_period_opt)
DateCollator->>DayPeriodString: day_period_string(hour, style)
DayPeriodString-->>DateCollator: label
DateCollator-->>Caller: parts including dayPeriod
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/intl/date_collator.rs (1)
1509-1518: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
dayPeriodthrough Temporal locale formatting.has_componentstill ignoresdayPeriod, and the finalformat_componentscall passesNone, so{ dayPeriod: 'long' }is treated as if no option was set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/intl/date_collator.rs` around lines 1509 - 1518, The Temporal locale formatting path is dropping dayPeriod, so `{ dayPeriod: 'long' }` is ignored. Update the relevant `has_component` logic and the `format_components` call in `date_collator.rs` to thread the `dayPeriod` option through instead of hardcoding `None`, using the existing `dayPeriod`-related symbols alongside `weekday_opt` and `era_opt`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/intl/date_collator.rs`:
- Around line 840-858: Update day_period_string in date_collator.rs to use the
correct English CLDR boundaries: map 0 to “midnight”, 1..=5 to “at night”,
6..=11 to “in the morning”, keep 12 as “noon”, and preserve the
afternoon/evening ranges. Also update the narrow-style handling so midnight
becomes “mi” and noon remains “n”, with the style override applied based on the
computed period string.
---
Outside diff comments:
In `@crates/perry-runtime/src/intl/date_collator.rs`:
- Around line 1509-1518: The Temporal locale formatting path is dropping
dayPeriod, so `{ dayPeriod: 'long' }` is ignored. Update the relevant
`has_component` logic and the `format_components` call in `date_collator.rs` to
thread the `dayPeriod` option through instead of hardcoding `None`, using the
existing `dayPeriod`-related symbols alongside `weekday_opt` and `era_opt`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 008e5e37-bbe0-490b-950e-34f7c72922f4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-runtime/src/intl/date_collator.rs
…eedback)
temporal_locale_string read every other DTF component option but never
dayPeriod, so has_component ignored it and the final format_components
call hardcoded None — Temporal.PlainDateTime(...).toLocaleString('en',
{dayPeriod: 'long'}) silently ignored the option. Thread it through like
weekday/era. Re-verified via test262_subset.py: same 45 remaining
failures, zero regressions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
dayPeriod-only subcluster of the test262 intl402/DateTimeFormat — 51 (self-contained worklist) #5840intl402/DateTimeFormatworklist.new Intl.DateTimeFormat('en', {dayPeriod: 'long'})(no other date/time component options) silently fell back to the defaultM/D/YYYYdate string in bothformatandformatToParts—dtf_primary_masknever counteddayPeriodtoward its time-dimension bit, and neitherformat_componentsnorbuild_parts_from_componentshad any dayPeriod rendering path (dayPeriod was previously only ever emitted as a byproduct of the AM/PM 12-hour clock).day_period_string(hour, style):enCLDR day-period boundaries at hour granularity (0-11 morning, 12 noon, 13-17 afternoon, 18-20 evening, 21-23 night;narrowabbreviates only "noon" → "n"), verified empirically againstnode.day_period_optparameter throughformat_components/build_parts_from_components: dayPeriod-only renders just the period text (one part informatToParts); combined with a numerichourit renders"<hour> <period>"([hour, literal " ", dayPeriod]informatToParts), replacing the default AM/PM suffix. Behavior is unchanged whenday_period_optisNone.Fixes:
prototype/format/dayPeriod-{long,narrow,short}-en.js,prototype/formatToParts/dayPeriod-{long,narrow,short}-en.js.Test plan
scripts/test262_subset.py --root vendor/test262 --dir intl402/DateTimeFormat(run underTZ=UTC— Perry's DTF hardcodes aUTCdefault timeZone whileDate's local-component constructor honors the real host offset, so a non-UTC dev box shows an unrelated pre-existing hour-shift onnew Date(y,m,d,h,...)-based cases; CI runs UTC): 6 target cases fixed, zero regressions (45 remaining failures = exactly the other 45 of the original 51-case worklist).cargo fmt --all -- --checkbash scripts/check_file_size.sh(date_collator.rs: 1951/2000 lines)scripts/run_gap_tests.sh— pre-existing gap failures unrelated to this change confirmed viagit stashbisection (e.g.test_gap_console_bare_globalreproduces identically on unmodifiedmain).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
dayPeriodlabels (morning/afternoon/evening and equivalents) more broadly, includingformatToPartsoutput.format/formatToPartshandledayPeriod-only cases and combineddayPeriod+ numeric hour cases more consistently.Bug Fixes
dayPeriodoption handling that could cause misclassification and incorrect rendering.Documentation