[PM-27098] Update some plurals to full-sentence forms to handle polypersonal agreement languages better #2240
Conversation
|
Claude finished @KatherineInCode's task in 2m 33s —— View job Code Review SummaryStatus: APPROVED - All localization changes are correctly implemented OverviewThis PR updates 4 plural string entries in
Changes AnalyzedAll 4 updated entries correctly preserve English output while enabling better translations:
Verification PerformedCorrectness: Verified each entry produces identical English output
Consistency: Approach matches existing entry Scope: Correctly targets only full-sentence entries; intentionally excludes simple noun plurals (XCharacters, XDays, XHours, etc.) used in menu selections Format: XML structure follows Apple's stringsdict format correctly with proper key naming and structure AssessmentSecurity: No security impact - localization only No issues found. This is a well-scoped localization improvement that will benefit international users. |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2240 +/- ##
==========================================
- Coverage 85.57% 84.27% -1.31%
==========================================
Files 1746 1989 +243
Lines 148371 163714 +15343
==========================================
+ Hits 126974 137973 +10999
- Misses 21397 25741 +4344 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
KatherineInCode
requested review from
a team and
matt-livefront
as code owners
|
@mKoonrad I'd love to get your eyes on this, also, to make sure I'm not missing anything, and these updates to the plurals look reasonable to you. |
| <string>%#@masterpasswordcharacters@.</string> | ||
| <key>masterpasswordcharacters</key> |
There was a problem hiding this comment.
🤔 Is it a requirement that this is fully in lowercase? If not, I feel like PascalCase is better/faster to read.
How do you feel about using the same as the main localization key here? in this case:
<string>%#@MasterPasswordMustBeAtLeastXCharactersLong@.</string>
<key>MasterPasswordMustBeAtLeastXCharactersLong</key>
Also, do you know if this <string> - <key> pair is presented in Crowdin somehow? Because if not, we could just simplify this to a generic way we can repeat in any localization:
<string>%#@pluralizedValue@.</string>
<key>pluralizedValue</key>
There was a problem hiding this comment.
The all-lowercase isn't required; that's just what I had done before. I'm fine if we want to move to camel case, though (which is, at least, what this guide to the format does). I'm hesitant to reuse a key in a substring, though, just to avoid any potential conflict.
I'm not inclined to do the generic way, as well, because of the potential issue with translation memory, which is one of the things we're trying to avoid by not putting the %#@items@ in the middle of a sentence, in addition to handling polypersonal agreement better.
It does show up in CrowdIn. Here's the vault items in Portuguese:
Also of interest here is that substrings end up as separate items to be translated—which makes sense, since my understanding is that you can nest substrings fairly deep if you really wanted to, and that provides a more-unified format for it, from a UX perspective. But that might want to inform how we structure things 🤔
There was a problem hiding this comment.
Makes sense.
However, IMO we should have a convention to make substring keys to avoid unwanted repetition/conflicts. That's why I was saying to use the same convention we use to build the localization key to build the substring key, i.e. join the substring value in Pascal Case as it's the best way to guarantee we won't repeat the same substring key in other localizations that are different. This ends up being the original localization key when the whole value is the substring as well.
Moreover, it could be something else that has some sort of convention we could use based off the substring value as well if the parent localization key has conflicts but I strongly believe we should have some sort of convention for it to avoid potential problems and be faster to add new keys without checking the others substring keys.
Thoughts?
There was a problem hiding this comment.
I agree that a convention would be helpful, though in a fair number of these, it's just the first sentence—which is what's already captured in the key for the whole phrase—that needs translated for number, so we might get substring keys that match the whole-phrase key 🤔 Though I suppose if we drop the X(/Y/Z/etc.) out of the key, that might accomplish what we want.
I'll swing through and update things CamelCase and see what I can do about establishing a more-obvious convention.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27098
📔 Objective
As noted in comments on a previous PR, the previous form of handling plurals in strings by breaking out just the plural had some issues. The first is that CrowdIn Translation Memory would cause issues with pluralized nouns being taken out of subject/object context, which doesn't work for languages that decline nouns differently for subjects and objects. The second is that languages with polypersonal agreement also require inflecting the verb congruently with the number of items, and we weren't allowing that.
Per that recommendation, I've pulled things out into full sentences, so that translators can provide better translations for the array of possibilities.
Nota bene: I elected even in circumstances where it is just a full sentence to leave the sentence-ending punctuation out of the pluralized forms. This allows us to remain consistent in multi-sentence situations, and for the base string to clearly indicate the sentence patterns. I'm not, however, wedded to that, I would just like to be consistent.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes