fix: keep zero plural form in Apple XLIFF export - #3789
Conversation
📝 WalkthroughWalkthroughUpdates Apple XLIFF plural mapping to preserve authored categories, include locale-required categories, and convert authored ChangesApple XLIFF plural export fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.kt (1)
231-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared plural-form helper here
AppleXliffExporter.kt:231-242— this duplicates the existingpopulateFormslogic inpluralFormsUtil.kt; call the helper instead of inlining the union/fallback code so Apple plural handling stays in one place.🤖 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 `@backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.kt` around lines 231 - 242, The plural-form handling in AppleXliffExporter.populateForms is duplicating the shared logic from pluralFormsUtil.kt. Update populateForms to delegate to the existing helper instead of inlining the union of getPluralFormsForLocale(languageTag) with forms.keys and the "other" fallback, so Apple plural behavior stays centralized and easier to maintain.
🤖 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.
Nitpick comments:
In
`@backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.kt`:
- Around line 231-242: The plural-form handling in
AppleXliffExporter.populateForms is duplicating the shared logic from
pluralFormsUtil.kt. Update populateForms to delegate to the existing helper
instead of inlining the union of getPluralFormsForLocale(languageTag) with
forms.keys and the "other" fallback, so Apple plural behavior stays centralized
and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 200c9117-a38a-4c96-91a1-09abd7e39465
📒 Files selected for processing (2)
backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.ktbackend/data/src/test/kotlin/io/tolgee/unit/formats/apple/out/AppleXliffFileExporterTest.kt
|
Deliberately not delegating to The shared helper stays correct as-is for the Android XML resources exporter, since Android plural resources really are constrained to CLDR quantities (noted in the PR description). Keeping this one local avoids changing behavior for that other caller. Happy to rename it to something more specific like |
|
Checked the shared helper before switching to it: |
|
On the suggestion to reuse I kept the change inside the Apple exporter on purpose instead of widening the shared helper. Apple's stringsdict/xcstrings accept whatever forms the author wrote, but the other formats that call |
|
Hi! Thanks for the PR ^^ For ICU, the proper way to handle the |
|
Checked the Apple side of this, since that's what the exporter feeds. Apple's plural docs use exactly this case as their worked example: English with an optional https://developer.apple.com/documentation/xcode/localizing-strings-that-contain-plurals I think that's the reason to keep this scoped to the Apple XLIFF exporter: in ICU terms an authored |
|
Hmm, in that case, we could probably keep all the standard categories (including |
|
That works, and it's a better shape than what I have. Filtering Folding On the second format: Happy to implement it the way you describe, with the |
|
Pushed the shape we talked through. No rush on your end — if the team lands somewhere different after you've talked it over, I'm happy to reshape it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/data/src/test/kotlin/io/tolgee/unit/formats/apple/out/AppleXliffFileExporterTest.kt (1)
311-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required TestData fixture flow.
Both tests create export views directly. Use a TestData class, save it with
testDataService, and clean it in@AfterEach.As per coding guidelines, “Use TestData classes for test setup following the pattern: create TestData instance, save with testDataService, and clean in
@AfterEach.”Also applies to: 356-369
🤖 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 `@backend/data/src/test/kotlin/io/tolgee/unit/formats/apple/out/AppleXliffFileExporterTest.kt` around lines 311 - 324, Update both affected tests to use the required TestData setup flow: create the appropriate TestData instance, persist it through testDataService before obtaining the exporter, and remove the direct export-view construction. Add or update `@AfterEach` cleanup to remove the saved fixture using the existing test cleanup pattern.Source: Coding guidelines
🤖 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
`@backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.kt`:
- Around line 246-247: Update the zero-form handling in the Apple XLIFF plural
export logic to check whether forms contains an explicit "zero" entry, rather
than whether result already contains "zero"; this must allow authored "=0" to
replace the CLDR zero fallback while preserving explicit zero forms. Add a
regression test using a locale with CLDR zero to verify "=0" maps to zero when
no explicit zero form exists.
---
Nitpick comments:
In
`@backend/data/src/test/kotlin/io/tolgee/unit/formats/apple/out/AppleXliffFileExporterTest.kt`:
- Around line 311-324: Update both affected tests to use the required TestData
setup flow: create the appropriate TestData instance, persist it through
testDataService before obtaining the exporter, and remove the direct export-view
construction. Add or update `@AfterEach` cleanup to remove the saved fixture using
the existing test cleanup pattern.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35187cf8-82c9-43e1-b59f-36e9faa22b6e
📒 Files selected for processing (2)
backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.ktbackend/data/src/test/kotlin/io/tolgee/unit/formats/apple/out/AppleXliffFileExporterTest.kt
| if ("zero" !in result) { | ||
| forms["=0"]?.let { result["zero"] = it } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check for an explicit zero form in forms.
For a locale with CLDR zero, result already contains zero from getPluralFormsForLocale. The current condition then drops an authored =0 and retains the other fallback. Check forms instead, so =0 replaces the fallback when no explicit zero exists. Add a regression test with a locale that has CLDR zero.
Proposed fix
- if ("zero" !in result) {
+ if ("zero" !in forms) {
forms["=0"]?.let { result["zero"] = it }
}The PR objective requires =0 to map to zero when no explicit zero form exists.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ("zero" !in result) { | |
| forms["=0"]?.let { result["zero"] = it } | |
| if ("zero" !in forms) { | |
| forms["=0"]?.let { result["zero"] = it } |
🤖 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
`@backend/data/src/main/kotlin/io/tolgee/formats/apple/out/AppleXliffExporter.kt`
around lines 246 - 247, Update the zero-form handling in the Apple XLIFF plural
export logic to check whether forms contains an explicit "zero" entry, rather
than whether result already contains "zero"; this must allow authored "=0" to
replace the CLDR zero fallback while preserving explicit zero forms. Add a
regression test using a locale with CLDR zero to verify "=0" maps to zero when
no explicit zero form exists.
8c1227a to
f14e1b8
Compare
Apple XLIFF export dropped any plural form that CLDR doesn't define for the target locale, so an authored zero case (e.g. English zero/one/other) never made it into the exported stringsdict/xcstrings. AppleXliffExporter now keeps every form the user actually wrote in addition to the locale's CLDR forms, matching what the xcstrings exporter already does. Fixes tolgee#3074
populateForms merged every authored form key wholesale, so an ICU =N exact match (e.g. =0) went into the XLIFF as a =N quantity key, which the Apple plural formats don't accept. Keep the locale's CLDR forms plus any authored named category (zero/one/two/few/many/other) and drop the =N keys. When a plural has an =0 but no explicit zero, surface it as zero, which is the closest thing stringsdict/xcstrings can express.
2 participants
Problem
Apple XLIFF export drops any plural form that CLDR doesn't define for the target locale. English only has
oneandotherin CLDR, so a key with azeroform (e.g. "No participants" / "1 Participant" / "%d Participants") gets exported withoutplural.zeroat all. The string is just gone from the file.AppleXliffExporter.handlePluralruns the converted forms throughpopulateForms, which builds its output set fromgetPluralFormsForLocale(languageTag)only. Any form outside that set gets dropped instead of exported. This is locale-specific behavior, not something Apple's formats actually require:AppleXcstringsExporter(the.xcstringsexporter) skips this filtering entirely and just writes out whatever formsformsResultcontains. That's why the reporter's.xcstringsworkaround kept the zero case while XLIFF/stringsdict export lost it.Change
AppleXliffExporter'spopulateFormsnow keeps every form that's actually present in the message, on top of the locale's CLDR forms (somanyfor Czech still gets filled in fromotherwhen the message doesn't specify it, same as before). This lines XLIFF export up with what the xcstrings exporter already does, so azeroform the user wrote makes it into the export regardless of whether the target locale's CLDR data includes it.I left
io.tolgee.formats.populateForms(used by the Android XML resources exporter) alone. Android's plural resources really are constrained to CLDR quantities, so filtering there is correct.Tests
Added a test to
AppleXliffFileExporterTestwith an English key carryingzero/one/otherforms and checked all three trans-units, includingplural.zero, show up in the exported XLIFF.No JDK on this machine, so I couldn't run
./gradlew :data:testlocally. I traced the fix by hand againstPluralsFormUtilTest(which already assertsgetPluralFormsForLocale("en")returns onlyone, other) and against howAppleXcstringsExporterbuilds its output. Would appreciate CI confirming the new test passes.Fixes #3074
Summary by CodeRabbit
Bug Fixes
=0translations that are mapped to the zero category when appropriate.Tests
=0plural forms.