Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(format-po-gettext): respect Plural-Forms header #2010

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timofei-iatsenko
Copy link
Collaborator

Description

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Examples update

Fixes # (issue)

Checklist

  • I have read the CONTRIBUTING and CODE_OF_CONDUCT docs
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation (if appropriate)

Copy link

vercel bot commented Aug 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 20, 2024 9:35am

Copy link

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.88 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.67 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

@garikkh
Copy link
Contributor

garikkh commented Aug 20, 2024

Might have to warn or hard error on compile if the msgstr does not match with nplurals header - for example:

msgid ""
msgstr ""
"Language: fr\\n"
"Plural-Forms: nplurals=3; plural=(n == 0 || n == 1) ? 0 : n != 0 && n % 1000000 == 0 ? 1 : 2;\\n"

#. js-lingui:icu=%7B0%2C+plural%2C+one+%7B%7Bcount%7D+day%7D+other+%7B%7Bcount%7D+days%7D%7D&pluralize_on=0
msgid "{count} day"
msgid_plural "{count} days"
msgstr[0] "{count} jour"
msgstr[1] "{count} jours"

will result in runtime errors:

TypeError: Cannot read properties of undefined (reading 'replace')
    at replaceOctothorpe (index.mjs:81:20)
    at plural (index.mjs:87:14)
    at eval (index.mjs:130:19)
    at Array.reduce (<anonymous>)
    at formatMessage (index.mjs:106:21)
    at eval (index.mjs:140:20)
    at I18n._ (index.mjs:319:6)
    at TransNoContext (react.1d406965.mjs:99:68)
    at renderWithHooks (react-dom.development.js:16305:18)
    at mountIndeterminateComponent (react-dom.development.js:20069:13)

@garikkh
Copy link
Contributor

garikkh commented Aug 21, 2024

Also some uncaught runtime errors if the locale isn't found in the cldr plurals lib.

Probably needs a cleaner error message, and also specifically for pseudo case. Following this guide there is this runtime error:

yarn i18n:compile                       
Compiling message catalogs…
node:internal/process/promises:391
    triggerUncaughtException(err, true /* fromPromise */);
    ^

RethrownError: Cannot read /home/MY_PROJECT/I18N/pseudo/lingui.po 
Original: TypeError: Cannot read properties of null (reading 'length')
    at getPluralCases (/home/MY_PROJECT/node_modules/@lingui/format-po-gettext/dist/po-gettext.cjs:22625:66)
    at Object.parse (/home/MY_PROJECT/node_modules/@lingui/format-po-gettext/dist/po-gettext.cjs:22722:25)
    at FormatterWrapper.read (/home/MY_PROJECT/node_modules/@lingui/cli/dist/api/formats/formatterWrapper.js:32:27)
    at async Catalog.read (/home/MY_PROJECT/node_modules/@lingui/cli/dist/api/catalog.js:153:16)
    at async /home/MY_PROJECT/node_modules/@lingui/cli/dist/api/catalog.js:157:77
    at async Promise.all (index 12)
    at async Catalog.readAll (/home/MY_PROJECT/node_modules/@lingui/cli/dist/api/catalog.js:157:9)
    at async getTranslationsForCatalog (/home/MY_PROJECT/node_modules/@lingui/cli/dist/api/catalog/getTranslationsForCatalog.js:6:22)
    at async Catalog.getTranslations (/home/MY_PROJECT/node_modules/@lingui/cli/dist/api/catalog.js:123:16)
    at async command (/home/MY_PROJECT/node_modules/@lingui/cli/dist/lingui-compile.js:26:30) {
  message: "Cannot read /home/MY_PROJECT/I18N

@timofei-iatsenko
Copy link
Collaborator Author

@garikkh could you take it over and finish this PR?

@garikkh
Copy link
Contributor

garikkh commented Sep 23, 2024

Most likely I will have some time this week to take over this PR.

@garikkh
Copy link
Contributor

garikkh commented Sep 26, 2024

Had a few fixes here: timofei-iatsenko/js-lingui@feature/respect-plural-forms-header...garikkh:js-lingui:feature/respect-plural-forms-header

tested the PO files my TMS generates, looks good.

One thing to note is that because the library uses Intl.PluralRules it technically isn't going to respect the Plural-forms header. Intl.PluralRules use CLDR plurals for which form to select. The benefit of this PR is that you can define either the gettext plural form or the CLDR plural form, and both will work.

Another note: I considered using either https://www.npmjs.com/package/cldr or https://www.npmjs.com/package/cldr-data instead of having to copy/paste a json file, but both were just as silly to use for this case.

Do you want me to make a new PR to lingui or to your branch?

@andrii-bodnar
Copy link
Contributor

@timofei-iatsenko @garikkh is there anything we can do to help move this PR forward?

Another note: I considered using either https://www.npmjs.com/package/cldr or https://www.npmjs.com/package/cldr-data instead of having to copy/paste a json file, but both were just as silly to use for this case.

💯

I also feel that committing these rules in raw format might not be the best approach. It might be more efficient and manageable to use a dependency for this instead.

@garikkh
Copy link
Contributor

garikkh commented Oct 7, 2024

@andrii-bodnar I linked a PR I made with @timofei-iatsenko fork as the base branch, just so it was a tiny bit neater. Wasn't sure if you guys wanted a separate PR.

timofei-iatsenko#1

I also feel that committing these rules in raw format might not be the best approach. It might be more efficient and manageable to use a dependency for this instead.

Agreed but the one being used here doesn't export the JSON and the maintainer doesn't want to add it to their exports. I don't remember the exact issues with the other two, so I could check again.

@garikkh
Copy link
Contributor

garikkh commented Oct 10, 2024

More details:

cldr-data is a great repository, but the plurals data is difficult to parse. each language has plurals defined like this:

      "cs": {
        "pluralRule-count-one": "i = 1 and v = 0 @integer 1",
        "pluralRule-count-few": "i = 2..4 and v = 0 @integer 2~4",
        "pluralRule-count-many": "v != 0   @decimal 0.0~1.5, 10.0, 100.0, 1000.0, 10000.0, 100000.0, 1000000.0, …",
        "pluralRule-count-other": " @integer 0, 5~19, 100, 1000, 10000, 100000, 1000000, …"
      },

And not exactly great to parse and extract actual samples from.

cldr is also same; its intended to be used with just the plural forms and the function:

> cldr.extractPluralClasses("cs")
[ 'one', 'few', 'many', 'other' ]
> cldr.extractPluralRuleFunction("cs")(1)
'one'
> cldr.extractPluralRuleFunction("cs")(100)
'other'

But other than that only provides raw xml for data:

<pluralRules locales="cs sk">
    <pluralRule count="one">i = 1 and v = 0 @integer 1</pluralRule>
    <pluralRule count="few">i = 2..4 and v = 0 @integer 2~4</pluralRule>
    <pluralRule count="many">v != 0   @decimal 0.0~1.5, 10.0, 100.0, 1000.0, 10000.0, 100000.0, 1000000.0, …</pluralRule>
    <pluralRule count="other"> @integer 0, 5~19, 100, 1000, 10000, 100000, 1000000, …</pluralRule>
</pluralRules>

And for the forms:

<pluralRanges locales="cs pl sk">
	<pluralRange start="one"   end="few"   result="few"/>
	<pluralRange start="one"   end="many"  result="many"/>
	<pluralRange start="one"   end="other" result="other"/>
	<pluralRange start="few"   end="few"   result="few"/>
	<pluralRange start="few"   end="many"  result="many"/>
	<pluralRange start="few"   end="other" result="other"/>
	<pluralRange start="many"  end="one"   result="one"/>
	<pluralRange start="many"  end="few"   result="few"/>
	<pluralRange start="many"  end="many"  result="many"/>
	<pluralRange start="many"  end="other" result="other"/>
	<pluralRange start="other" end="one"   result="one"/>
	<pluralRange start="other" end="few"   result="few"/>
	<pluralRange start="other" end="many"  result="many"/>
	<pluralRange start="other" end="other" result="other"/>
</pluralRanges>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants