Added count param to phrase param and return plural form accordingly #48229

ZhenjaHorbach · 2024-08-28T22:05:58Z

Details

This PR added a count parameter to the phrase parameter to return the plural form when necessary. Also In this PR we
updated the types to support PluralizeValues and refactored the code so that all translation functions now use a single object containing all the necessary parameters

Fixed Issues

#38614
PROPOSAL:

Tests

Open App
Open few screens and verify that the translations are displayed correctly.

Verify that no errors appear in the JS console

Offline tests

Open App
Open few screens and verify that the translations are displayed correctly.

QA Steps

Open App
Open few screens and verify that the translations are displayed correctly.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

android.mov

Android: mWeb Chrome

android-web.mov

iOS: Native

ios.mov

iOS: mWeb Safari

ios-web.mov

MacOS: Chrome / Safari

web.mov

MacOS: Desktop

desktop.mov

shubham1206agra · 2024-08-29T15:40:27Z

src/languages/types.ts


-type TranslationBase = {[key: string]: TranslationBaseValue | TranslationBase};
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+type ArgumentType<T, R> = T extends (arg: infer A, ...args: any[]) => R ? A : unknown;


@ZhenjaHorbach Can you explain this please?

It's just helper type
Which I used for checking first parameter

And basically it is used to determine the type of the first parameter
And if the first parameter is not an object
We will get a TS error

melvin-bot · 2024-08-30T16:00:28Z

@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

ZhenjaHorbach · 2024-09-03T11:52:39Z

@shubham1206agra
Could you please check PR ?
Thanks

dubielzyk-expensify · 2024-09-04T00:22:39Z

Assume this needs no design review? If so, let me know and I'll check it out :)

ZhenjaHorbach · 2024-09-04T05:59:06Z

Assume this needs no design review? If so, let me know and I'll check it out :)

Oh sorry 😅
Looks like after I updated the branch to the last main Design label appeared
But in this PR there are no style changes

But thanks !

shubham1206agra · 2024-09-05T07:42:42Z

@ZhenjaHorbach Have you enforced the single parameter condition in function in translation? Since I was able to add another parameter without any errors.

ZhenjaHorbach · 2024-09-05T07:45:04Z

@shubham1206agra
Yes
Could you provide example ?

ZhenjaHorbach · 2024-09-05T07:53:39Z

Here is an example

ZhenjaHorbach · 2024-09-25T16:22:12Z

@iwiznia
We have only Changed files ESLint check issues
But we can ignore them
Plus they are not related to our changes

iwiznia

So we are removing the possibility to call translate('bla', 'param', 'other'). That's fine, but my fear is that other unmerged PRs could be using that format and we would not catch that, right?

Can you update the links in https://github.com/Expensify/App/#internationalization and add a small explanation of the pluralization feature?

iwiznia · 2024-09-25T18:27:46Z

src/libs/Localize/index.ts

-    phraseKey: TKey,
-    fallbackLanguage: 'en' | 'es' | null = null,
-    ...phraseParameters: PhraseParameters<Phrase<TKey>>
+    path: TPath,


FWIW I think I liked more phraseKey then path, NAB

iwiznia · 2024-09-25T18:28:48Z

src/libs/Localize/index.ts

-            return translatedPhrase(...phraseParameters);
+            /**
+             *
+             * is Plain object is for checking if the phraseTranslated output


is Plain object is is this referring to a method or what?

phraseTranslated

I think you mean translatedPhrase?

iwiznia · 2024-09-25T18:28:58Z

src/libs/Localize/index.ts

+            /**
+             *
+             * is Plain object is for checking if the phraseTranslated output
+             * is an object then further check if it include the count param or not


Suggested change

* is an object then further check if it include the count param or not

* is an object then further check if it includes the count param or not

iwiznia · 2024-09-25T18:29:46Z

src/libs/Localize/index.ts

+             *
+             * is Plain object is for checking if the phraseTranslated output
+             * is an object then further check if it include the count param or not
+             * OR before checking the plain object output, we can check if we have the count


Not 100% what exactly is the counterpart of this OR... the last if here, both last ifs?

Oh
Sorry
These were draft comments 😅
I agree that they should be updated

iwiznia · 2024-09-25T18:32:37Z

src/libs/Localize/index.ts

+            }
+
+            const phraseObject = parameters[0];
+            if (phraseObject && typeof phraseObject === 'object' && 'count' in phraseObject && typeof phraseObject.count === 'number') {


I don't think we need to check 'count' in phraseObject since if count is not defined, typeof would return undefined and this code would work anyway

I think we need some additional clarification to prevent Property 'count' does not exist on type 'object'.

Can't we change the type of phraseObject to allow for an optional count?

I think we can do it
But I'm not sure that is a good idea update TranslationParameters type for count
Plus we use such a condition to write count key into phraseObject

But I'm not sure that is a good idea update TranslationParameters type for count

Why?

Plus we use such a condition to write count key into phraseObject

Which we could remove if we added it as an optional param, no?

iwiznia · 2024-09-25T19:17:51Z

src/libs/Localize/index.ts

+            if (phraseObject && typeof phraseObject === 'object' && 'count' in phraseObject && typeof phraseObject.count === 'number') {
+                const pluralRule = new Intl.PluralRules(language).select(phraseObject.count);
+
+                const pluralResult = pluralRule in translateResult && translateResult[pluralRule];


Do we need the pluralRule in translateResult I think the code would work exactly the same without it, no?

iwiznia · 2024-09-25T19:19:14Z

src/libs/Localize/index.ts

+                return translateResult.other(phraseObject.count);
+            }
+
+            throw new Error(`Invalid plural form for '${path}'`);


Throw early (ie: invert this if condition and throw)

I think it's ok !
Because this error symbolizes that all the previous conditions were not pass
As esle

What all previous conditions? There's only one of them.
We prefer early throw/return in general, as it makes the code easier to read and follow

iwiznia · 2024-09-25T19:52:07Z

Oh, can you also pick some existing translation, for example these ones and change them to use the new pluralization methods?
Just to leave as an example in real code.
I think I will create a new issue later to remove all usages of Str.pluralize which should not be needed anymore

ZhenjaHorbach · 2024-09-25T20:39:15Z

So we are removing the possibility to call translate('bla', 'param', 'other'). That's fine, but my fear is that other unmerged PRs could be using that format and we would not catch that, right?

Can you update the links in https://github.com/Expensify/App/#internationalization and add a small explanation of the pluralization feature?

Actually a good question
But I think that this is not critical
Since in this case only TS will break on main after merging PR with wrong format of translates

shubham1206agra · 2024-09-26T01:24:39Z

Oh, can you also pick some existing translation, for example these ones and change them to use the new pluralization methods? Just to leave as an example in real code. I think I will create a new issue later to remove all usages of Str.pluralize which should not be needed anymore

This will be done by me in #45892

iwiznia · 2024-09-26T14:20:30Z

Since in this case only TS will break on main after merging PR with wrong format of translates

Not sure I follow. Are you saying that if you call translate(key, 'one', 'two') it would still work?

This will be done by me in #45892

Ah nice!

ZhenjaHorbach · 2024-09-26T15:06:17Z

Since in this case only TS will break on main after merging PR with wrong format of translates

Not sure I follow. Are you saying that if you call translate(key, 'one', 'two') it would still work?

Yes
Since this pattern does not affect the new pluralization in any way, it will work the same as before
Provided that the translation will look like this

key: (one, two)=>${one}${two}

But this is expected
Since in the current typing on main in the case of a translation function we return only a string

iwiznia · 2024-09-26T15:43:22Z

Yes

Ah nice, thanks for checking and explaining!

ZhenjaHorbach · 2024-09-26T15:47:31Z

Ah nice, thanks for checking and explaining!

And I fixed other comments 😃
So if you have no more questions
We can move on !

iwiznia · 2024-09-26T15:55:36Z

src/languages/en.ts

-            deleteRates: ({count}: DistanceRateOperationsParams) => `Delete ${Str.pluralize('rate', 'rates', count)}`,
+            deleteRates: () => ({
+                one: 'Delete rate',
+                other: () => `Delete rates`,


Why is this a function?

Since it will be easy to implement, and we need to pass the count somehow.

But same as the others, you could just have a string here right?

Yes
I also thought about it 5 minutes ago 😅
I will merge changes soon

Now it will work!

iwiznia · 2024-09-26T15:57:44Z

README.md

+  messages: () => ({
+      zero: 'No messages',
+      one: 'One message',
+      two: 'Two messages',
+      few: (count) => `${count} messages`,
+      many: (count) => `You have ${count} messages`,
+      other: (count) => `You have ${count} unread messages`,
+  })
+


Oh crap, did not really notice this before... does the object containing the messages always need to be a function or can it also be:

Suggested change

messages: () => ({

zero: 'No messages',

one: 'One message',

two: 'Two messages',

few: (count) => `${count} messages`,

many: (count) => `You have ${count} messages`,

other: (count) => `You have ${count} unread messages`,

})

messages: {

zero: 'No messages',

one: 'One message',

two: 'Two messages',

few: (count) => `${count} messages`,

many: (count) => `You have ${count} messages`,

other: (count) => `You have ${count} unread messages`,

}

There's really no need for a function there...

No, since it will be impossible to differentiate between messages and messages.one translation key in a flat object.

This implementation looks like any other object with translations
And not a specific translation with plural forms

No, since it will be impossible to differentiate between messages and messages.one translation key in a flat object.

I don't get this

This implementation looks like any other object with translations
And not a specific translation with plural forms

Yeah, so?

Anyway, it would be nice if we supported the simple object notation too, but not going to block on that since this is a big PR, we've been waiting a long time for it and adding the function is not that bad.

iwiznia · 2024-09-26T16:25:22Z

src/languages/es.ts

-            deleteRates: ({count}: DistanceRateOperationsParams) => `Eliminar ${Str.pluralize('tasa', 'tasas', count)}`,
+            deleteRates: () => ({
+                one: 'Eliminar tasa',
+                other: () => `Eliminar tasas`,


ZhenjaHorbach · 2024-09-26T16:59:17Z

@iwiznia
Sorry about the last commit 😅
I fixed that comment

If everything is ok, could you approve it again and merge it?

iwiznia · 2024-09-26T17:19:40Z

We are skipping ESLint checks because this PR is touching 76 unrelated files and fixing all the previously existing lint errors would make this PR riskier.

Thanks so much for this, great job!!!

melvin-bot · 2024-09-26T17:19:52Z

@iwiznia looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2024-09-26T17:30:34Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2024-09-27T17:58:23Z

🚀 Deployed to staging by https://github.com/iwiznia in version: 9.0.41-0 🚀

platform	result
🤖 android 🤖	cancelled 🔪
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

ZhenjaHorbach added 7 commits August 29, 2024 00:04

update TranslationBase and make few changes

736b250

fix conflicts

9d42d55

update TranslationBase

e830bb5

update translations PART-1

09f51d4

update translations PART-2

a237886

update translations PART-3

d0bf65b

update translations PART-4

41f0334

ZhenjaHorbach mentioned this pull request Aug 29, 2024

Added pluralization system for lang translations #45892

Merged

49 tasks

shubham1206agra reviewed Aug 29, 2024

View reviewed changes

ZhenjaHorbach marked this pull request as ready for review August 30, 2024 16:00

ZhenjaHorbach requested a review from a team as a code owner August 30, 2024 16:00

melvin-bot bot requested a review from shubham1206agra August 30, 2024 16:00

melvin-bot bot removed the request for review from a team August 30, 2024 16:00

ZhenjaHorbach added 3 commits August 31, 2024 12:08

Merge branch 'main' into pluralizing_localization-v2

078a5ad

update TranslationBase type

b70f5e4

fix conflicts

f31c7f2

ZhenjaHorbach requested a review from a team as a code owner September 3, 2024 07:06

fix conflicts

af67cd0

fix conflicts

eab923d

update new translations

5d1a70c

ZhenjaHorbach added 3 commits September 5, 2024 10:10

update TranslationBaseValue

41a3199

revert some changes

8ebf751

revert some changes

273c8e1

update branch

ffcd48e

iwiznia requested changes Sep 25, 2024

View reviewed changes

iwiznia removed the request for review from a team September 25, 2024 19:49

ZhenjaHorbach added 4 commits September 25, 2024 23:04

fix some comments

480bdda

update readme

1ebfb54

update readme x2

27b7056

update localize comment

7638a4c

fix comments

12b3b41

iwiznia reviewed Sep 26, 2024

View reviewed changes

iwiznia previously approved these changes Sep 26, 2024

View reviewed changes

refactor plural forms

a36a304

ZhenjaHorbach dismissed iwiznia’s stale review via a36a304 September 26, 2024 16:54

iwiznia approved these changes Sep 26, 2024

View reviewed changes

iwiznia merged commit 04b82bc into Expensify:main Sep 26, 2024
17 of 18 checks passed

melvin-bot bot added the Emergency label Sep 26, 2024

iwiznia removed the Emergency label Sep 26, 2024

github-actions bot mentioned this pull request Sep 27, 2024

Deploy Checklist: New Expensify 2024-09-27 #49855

Open

	* is an object then further check if it include the count param or not
	* is an object then further check if it includes the count param or not

Added count param to phrase param and return plural form accordingly #48229

Added count param to phrase param and return plural form accordingly #48229

Conversation

ZhenjaHorbach commented Aug 28, 2024 • edited by iwiznia Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melvin-bot bot commented Aug 30, 2024

ZhenjaHorbach commented Sep 3, 2024

dubielzyk-expensify commented Sep 4, 2024

ZhenjaHorbach commented Sep 4, 2024

shubham1206agra commented Sep 5, 2024

ZhenjaHorbach commented Sep 5, 2024

ZhenjaHorbach commented Sep 5, 2024 • edited Loading

ZhenjaHorbach commented Sep 25, 2024

iwiznia left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia commented Sep 25, 2024

ZhenjaHorbach commented Sep 25, 2024 • edited Loading

shubham1206agra commented Sep 26, 2024

iwiznia commented Sep 26, 2024

ZhenjaHorbach commented Sep 26, 2024 • edited Loading

iwiznia commented Sep 26, 2024

ZhenjaHorbach commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1206agra Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

ZhenjaHorbach Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach commented Sep 26, 2024

iwiznia commented Sep 26, 2024

melvin-bot bot commented Sep 26, 2024

OSBotify commented Sep 26, 2024

github-actions bot commented Sep 27, 2024

ZhenjaHorbach commented Aug 28, 2024 •

edited by iwiznia

Loading

ZhenjaHorbach commented Sep 5, 2024 •

edited

Loading

iwiznia left a comment •

edited

Loading

ZhenjaHorbach commented Sep 25, 2024 •

edited

Loading

ZhenjaHorbach commented Sep 26, 2024 •

edited

Loading

ZhenjaHorbach commented Sep 26, 2024 •

edited

Loading

ZhenjaHorbach Sep 26, 2024 •

edited

Loading

shubham1206agra Sep 26, 2024 •

edited

Loading

ZhenjaHorbach Sep 26, 2024 •

edited

Loading