[No QA] Preserve function wrappers for incremental plural translations - #99222
Conversation
Collapse incremental translation paths at the nearest function-valued
property so keys like codingRules: (params) => ({one, other}) are
captured and re-injected as a whole instead of flattened to a plain object.
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
startsWith treated iou.deleteReport as a prefix of iou.deleteReportConfirmation, so sibling plural keys were retranslated. Require an exact match or a following dot.
This comment has been minimized.
This comment has been minimized.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e51f20723d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (isFunctionValuedProperty(current)) { | ||
| pathParts.length = 0; |
There was a problem hiding this comment.
When a diff changes only one branch of an existing function-valued plural, clearing the accumulated path collapses that branch to the enclosing function key. shouldTranslatePath() then selects every string beneath that key, and the incremental transformer replaces the entire target-language function initializer. For example, correcting only codingRules.one also retranslates and overwrites the existing codingRules.other, potentially discarding a curated translation and incurring unnecessary provider cost. Preserve the wrapper as the injection boundary without retranslating unchanged branches, or reserve whole-function replacement for keys absent from the target file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I thoroughly explored how to fix this, but the code complexity to solve this edge case is really not worth it, especially given that even if it does occur, there's no guarantee it would actually be a significant problem. (i.e: the likelihood the retranslation is meaningfully worse than the previous one is slim)
47e5251 to
e51f207
Compare
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
3 participants
Explanation of Change
The incremental translation path built a leaf path such as
codingRules.otherfor strings inside a function-valued plural key.extractTranslatedNodesthen captured only the leaf initializers, andinjectDeepObjectValuereassembled them as a plain object (codingRules: {one, other}), dropping the(params) => (...)wrapper.buildDotNotationPathnow collapses at the nearest function-valued property assignment, so the whole function is the translatable unit. Leaf strings inside it are still translated; the wrapper and params are preserved.The secondary CLDR plural-forms concern from the issue is out of scope and should be a follow-up.
Fixed Issues
$ #94343
PROPOSAL: #94343 (comment)
Tests
Added unit tests.
Offline tests
None.
QA Steps
None.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A — tooling-only change
Android: mWeb Chrome
N/A — tooling-only change
iOS: Native
N/A — tooling-only change
iOS: mWeb Safari
N/A — tooling-only change
MacOS: Chrome / Safari
N/A — tooling-only change