Add macros for the remaining ICU argument types - #82
Conversation
…are numeric select keys
🦋 Changeset detectedLatest commit: 72e2648 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
examples
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
WalkthroughThe PR adds ICU number, date, and time macros, plural offsets, numeric ChangesICU formatting support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@examples/react/src/board.ts`:
- Line 71: Update the `sprintEndsAt` initialization to preserve 17:00 local time
across daylight-saving transitions: create the date, set its hours to 17:00,
then advance it by three calendar days with `setDate` instead of adding
milliseconds.
In `@examples/react/src/components/board.tsx`:
- Around line 80-85: Update the formatter extraction comments in
examples/react/src/components/board.tsx at lines 80-85 to accurately document
the complete, sprintEndsAt date, and sprintEndsAt time placeholders. Also update
the comments in examples/vanilla/src/main.ts at lines 106-109 to identify the
queue message as say.plural and describe say.number separately; make no other
code changes.
- Around line 100-107: Update the plural branches in
examples/react/src/components/board.tsx (lines 100-107) and
examples/vanilla/src/main.ts (lines 111-119) to match total-count selection by
changing the exact zero branches to exact one branches, while preserving
offset-based “other” wording. Update every matching affected PO entry’s msgid
and msgstr from =0 to =1.
In `@packages/config/src/features/messages/format.test.ts`:
- Around line 55-59: Update validateArgumentStyle to reject the currency style
for number arguments, preventing unsupported formatter output. Change the test
around validateArgumentStyle to call it with number instead of date while
preserving the expected Invalid date style assertion only if the
implementation’s existing error message requires it.
In `@packages/config/src/features/messages/identifier.ts`:
- Around line 55-57: Update the validation flow around getBranchCase and
BRANCH_PATTERN to reject exact-value keys matching =\d+ when kind is 'select',
while preserving existing numeric exact-value handling for other branch types.
Add a regression test asserting validateBranchIdentifier('select', '=0') is
rejected.
In `@packages/config/src/features/messages/types.ts`:
- Around line 30-32: Restrict ArgumentFormat.type to the supported ArgumentType
union instead of a general string. At the ICU conversion boundary, validate type
and style combinations and reject offset values unless the argument kind permits
them, preventing offsets on select. Add direct-construction tests for the
message classes covering unsupported types/styles and invalid
ChoiceMessage.offset usage.
In `@packages/integration/src/types.ts`:
- Around line 28-31: Update the selector parameter types in Say.select and
Say.Select to accept string | number | Named<string | number>, matching
SelectOptions and allowing bare numeric ICU cases; add type coverage confirming
numeric selectors compile for both macro APIs.
In `@website/content/core-concepts/messages.mdx`:
- Around line 219-222: Update the `select` documentation to state that selector
values are compared as exact strings, so hyphenated values such as “sold-out”
must be normalized before calling `say.select`, or replaced with ICU-compatible
source discriminants; otherwise they fall through to `other`.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0bbfe92-c1ab-46fc-b1cc-262523e89b17
📒 Files selected for processing (32)
.changeset/brave-pumas-invent.md.changeset/olive-moons-shake.mdexamples/react/src/board.tsexamples/react/src/components/board.tsxexamples/react/src/locales/en.poexamples/react/src/locales/fr.poexamples/react/src/locales/ja.poexamples/react/src/locales/pl.poexamples/vanilla/src/library.tsexamples/vanilla/src/locales/en.poexamples/vanilla/src/locales/fr.poexamples/vanilla/src/locales/ja.poexamples/vanilla/src/locales/pl.poexamples/vanilla/src/main.tspackages/config/src/features/messages/convert.test.tspackages/config/src/features/messages/convert.tspackages/config/src/features/messages/format.test.tspackages/config/src/features/messages/format.tspackages/config/src/features/messages/identifier.test.tspackages/config/src/features/messages/identifier.tspackages/config/src/features/messages/index.tspackages/config/src/features/messages/types.tspackages/integration-react/src/runtime/index.tspackages/integration/src/runtime.test.tspackages/integration/src/runtime.tspackages/integration/src/types.tspackages/transform-js/src/parser.test.tspackages/transform-js/src/parser.tspackages/transform-jsx/src/parser.test.tspackages/transform-jsx/src/parser.tswebsite/content/core-concepts/messages.mdxwebsite/content/integrations/react.mdx
|
@greptile review |
|
✅ Action performedReview finished.
|
Greptile SummaryThe PR adds number, date, and time formatting fragments plus plural offsets across the configuration model, JavaScript and JSX transforms, runtime typings, examples, tests, and documentation.
Confidence Score: 5/5The PR appears safe to merge with no concrete blocking or independently actionable non-blocking issues identified. The formatted-argument pipeline preserves ICU formats while sharing raw descriptor values correctly, literal offsets are consistently parsed across JavaScript and JSX, and numeric select keys are serialized according to the runtime formatter’s contract. Reviews (1): Last reviewed commit: "Address CodeRabbit review on ICU argumen..." | Re-trigger Greptile |
@saykit/config
@saykit/format-json
@saykit/format-po
saykit
@saykit/carbon
@saykit/react
babel-plugin-saykit
unplugin-saykit
@saykit/transform-js
@saykit/transform-jsx
commit: |
|
The preview build of |
github-actions
Bot
removed
the
preview
1 participant
Closes #74.
Adds the missing ICU argument types as macros:
say.number,say.date,say.time(and<Say.Number>,<Say.Date>,<Say.Time>), plus pluraloffset.Approach
These are fragments, not whole messages, and the existing pipeline already handles that for free:
parseCallExpressionreturns aCompositeMessage,parseExpressionrecurses into nestedsaymacros,convertMessageToIcuflattens composite children, andgenerateChildExpressionsrecurses. Sosay`You have ${say.number(n)} items`needed no new nesting machinery, only new call-kind recognition.ArgumentMessagegains an optionalformat: { type, style? }rather than getting a newFormatMessagesibling.assignSequenceIdentifiers,collectAssignedIdentifiers, andgenerateChildExpressionsall already branch oninstanceof ArgumentMessage, so all three needed zero changes.Styles stay ICU strings rather than
Intloption objects: the catalogue is the source of truth, and only the ICU spelling round-trips back out of it.Two findings from probing the runtime formatter
currencyis excluded, and that isn't a style preference.{n, number, currency}doesn't error, it silently formats as{$n}with a stderr warning, because MF1 has nowhere to write the currency code. Shipping it would have been worse than omitting it. Literal patterns (#,##0.00) do work, so those are accepted. Documented with theIntl.NumberFormatworkaround.=0inselectwas backwards. The issue lists numeric select keys as "emitted bare, not as=n". Both halves were off:getBranchCasedid prefix=, andvalidateBranchIdentifierthen rejected it, sosay.select(tier, { 0: 'Free' })was a hard build error. Probing settled it,=0is a parse error underselectwhile a bare0matches both0and'0'.getBranchCasenow takes the kind, and the select-rejection branch is gone.Also here
A pre-existing bug, fixed because I was editing the same files: the React example's
fr/pl/jacatalogues still carriedAssigned to <0>{1}</0>from before thesay-tagrename. Ids are content hashes, so it no longer resolved and the example threw in every non-English locale. Happy to split it out if you'd rather.Docs
Moved the new section after Select (it had orphaned Ordinals and Select from the branching group), and took a pass over the whole page. Net ~57 lines shorter despite the additions: merged the duplicated CLDR-categories explanation in Plurals, dropped the Context example repeated verbatim in Best practices, and collapsed the per-component JSX listing. Two genuine gaps filled, Ordinals presented numeric keys and CLDR categories as equivalent (
1: '#st'leaves 21st as "21th"), and{' '}inside a<Say>extracts as a placeholder, which I hit writing the example.Not included
Skeletons stay blocked on the formatter rejecting
::.spellout, RBNFordinal, andchoiceare now documented as explicit "no"s.Verification
pnpm check,oxlint, and all 10 package builds clean.saykit extracton both examples, the new ICU round-trips out of source.enand format cleanly through the realmf1ToMessage.🤖 Generated with Claude Code
Summary by CodeRabbit