Add CLDR plural categories with a legacy translation path - #500
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe localization service now selects plural translation forms using language-specific cardinal rules. Numeric translation arguments use this plural lookup when they convert to decimal. The previous numeric-key behavior remains available through ChangesPlural Localization
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant TranslationFunctions
participant ILocalizationService
participant EmbeddedYamlLocalizationService
participant PluralRules
Caller->>TranslationFunctions: Call t with numeric count
TranslationFunctions->>ILocalizationService: TranslatePlural with key, count, and language
ILocalizationService->>EmbeddedYamlLocalizationService: Resolve plural translation
EmbeddedYamlLocalizationService->>PluralRules: Select category for language and count
PluralRules-->>EmbeddedYamlLocalizationService: Return category
EmbeddedYamlLocalizationService-->>TranslationFunctions: Return matching form or fallback
TranslationFunctions-->>Caller: Return translated text
Merge Risk: ⚪ Minimal · up to No actionable regression is established for this change; it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The checked paths remain application-local translation and display operations. Existing numeric-key consumers retain an explicit compatibility path. No new privileged operation or externally reachable attack path was established, but coverage of all downstream consumers is incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 7 files. (2 skipped: 2 unsupported.) ✨ 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. I’m a rabbit; I count each hop, Comment |
434c164 to
ccbbe06
Compare
patchzyy
left a comment
There was a problem hiding this comment.
reviewed this against #499. the existing tests pass locally: 580 unit tests and 19 ui tests.
[p2] keep the locale region when choosing the plural category.
with a portuguese catalog containing one and other, t("pt-pt.items", count: 0) returns the one form. the rule selector correctly returns other for that locale, but the translation service normalizes pt-pt to pt before calling it, so the regional rule is never reached through the translation api.
please use the normalized language for catalog lookup while retaining the full locale for plural selection, and add an integration test through t() or the service. the current regional test only calls the rule selector directly. the cldr rules distinguish these locales.
location: embeddedyamllocalizationservice.cs, the language normalization at the start of plural translation.
i confirmed this with an extra embedded fixture and a failing regression test in an isolated copy of this commit.
Purpose of this PR:
Keep global t() and add count-based category lookup for zero/one/two/few/many/other using CLDR cardinal rules for the selectable languages. Named count arguments and positional counts are supported; fallback recomputes the English plural category.
Add t_legacy() for exact numeric/.n variants and move the current numeric-key callers, including tTime(), to it. Imported production YAML is unchanged. Test-only embedded translations exercise the new categories.
Targets main after the settings layers were merged. This is PR 1/3 of the remaining localization stack; works without live localization.
How to Test:
dotnet test WheelWizard.sln580 tests passed (561 unit, 19 headless UI), including decimal counts, language-specific rules, English fallback, named/positional calls, and legacy behavior.
What Has Been Changed:
See the focused implementation above. The existing settings JSON contract and imported translation sheets are preserved.
Related Issue Link:
No linked issue. PR 1/3 of native GitHub stack #508 (#500 → #501 → #502).
Checklist before merging
Summary by CodeRabbit