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

Wrong Plural-Forms for Brazilian Portuguese #67

Closed
rankida opened this issue Oct 9, 2017 · 19 comments
Closed

Wrong Plural-Forms for Brazilian Portuguese #67

rankida opened this issue Oct 9, 2017 · 19 comments

Comments

@rankida
Copy link
Contributor

rankida commented Oct 9, 2017

Hi. Thanks for this great tools!

I have encountered an issue when trying to generate a PO file for Brazilian Portugal. This may be user, error, but this is what I see:

I run

./node_modules/.bin/i18next-conv -l pt_br -s hello.json -t hello.po

where hello.json is nothing more than { "Hello": "Olá" } and I get a PO file with

"Plural-Forms: nplurals=2; plural=(n != 1)\n"

The issue is that for Brazillian Portuguese it should be nplurals=( n > 1), and it is correct in plurals.json here https://github.com/i18next/i18next-gettext-converter/blob/master/src/lib/plurals.js#L494-L498

I have tried using pt-BR or pt-br but they both give me the regular Portuguese plural forms.

I suspect the problem is down to this line

const ext = plurals.rules[locale.replace('_', '-').split('-')[0]];

const ext = plurals.rules[locale.replace('_', '-').split('-')[0]];
@jamuhl
Copy link
Member

jamuhl commented Oct 9, 2017

did you try ./node_modules/.bin/i18next-conv -l pt-BR -s hello.json -t hello.po ?!?

notice pt_br vs pt-BR

@rankida
Copy link
Contributor Author

rankida commented Oct 9, 2017

Yes, ./node_modules/.bin/i18next-conv -l pt-BR -s hello.json -t hello.po gave the same

msgid ""
msgstr ""
"Project-Id-Version: i18next-conv\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=utf-8\n"
"Content-Transfer-Encoding: 8bit\n"
"Plural-Forms: nplurals=2; plural=(n != 1)\n"
"POT-Creation-Date: 2017-10-09T14:12:24.391Z\n"
"PO-Revision-Date: 2017-10-09T14:12:24.391Z\n"
"Language: pt-BR\n"

msgid "Hello"
msgstr "Olá"

@rankida
Copy link
Contributor Author

rankida commented Oct 9, 2017

Sorry, I linked to the wrong file. I suspect the issues is here

function getPluralArray(domain, translation) {
const ext = plurals.rules[domain.replace('_', '-').split('-')[0]];
const pArray = [];
for (let i = 0, len = translation.plurals.length; i < len; i += 1) {
const plural = translation.plurals[i];
pArray.splice(getGettextPluralPosition(ext, plural.pluralNumber - 1), 0, plural.value);
}
pArray.splice(getGettextPluralPosition(ext, translation.pluralNumber - 1), 0, translation.value);
return pArray;
}
, specifically this line
const ext = plurals.rules[domain.replace('_', '-').split('-')[0]];

@jamuhl
Copy link
Member

jamuhl commented Oct 9, 2017

right should convert _ and work anyway.

but https://github.com/i18next/i18next-gettext-converter/blob/master/src/lib/plurals.js#L489

the rule taken is pt as the code splits at - and takes only pt to lookup plural rule

looking at http://www.unicode.org/cldr/charts/29/supplemental/language_plural_rules.html#pt i guess the rule plurals: '(n > 1)', would be correct

@jamuhl
Copy link
Member

jamuhl commented Oct 9, 2017

but looking at: http://docs.translatehouse.org/projects/localization-guide/en/latest/l10n/pluralforms.html plural rules for portugues and portugues in brasil are different...

as i speak neither i'm not sure which source is right

@rankida
Copy link
Contributor Author

rankida commented Oct 9, 2017

http://docs.translatehouse.org/projects/localization-guide/en/latest/l10n/pluralforms.html was my reference source also.

Our Brazilian translators gave us a PO file with "Plural-Forms: nplurals=2; plural=(n > 1);\n" which is how this came up. My understanding is that this differs from regular Portuguese which is n != 1

A quick google seems to confirm it here https://doc.qt.io/archives/qq/qq19-plurals.html

Case in point: In French and Brazilian Portuguese (but not international Portuguese, interestingly enough), the singular form is used in conjunction with 0 (e.g., "0 maison", not "0 maisons")

So I think your info in https://github.com/i18next/i18next-gettext-converter/blob/master/src/lib/plurals.js is correct, it is just the split on - that results in pt always being used

    pt: {
      name: 'Portuguese',
      nplurals: 2,
      plurals: '(n != 1)',
    },
    pt_br: {
      name: 'Brazilian Portuguese',
      nplurals: 2,
      plurals: '(n > 1)',
    },

Thanks for your quick reply 👍

@jamuhl
Copy link
Member

jamuhl commented Oct 9, 2017

honestly wasn't aware that regional locals of a language even goes on changing the pluralforms - that's the reason why - even the CLDR which is the base of most localization tools does not differ there.

@perrin4869 a solution would be to test first if a rule exists for a full match eg. pt-BR before taking pt - what do you think?

@jamuhl
Copy link
Member

jamuhl commented Oct 9, 2017

ok seems http://www.unicode.org/cldr/charts/26/supplemental/language_plural_rules.html new default is now pt = pt-BR and a special treatment for pt-PT ;)

does it make sense...? Think i need to update this on i18next as well...:(

@rankida
Copy link
Contributor Author

rankida commented Oct 9, 2017

I can try and check with the translator we used. I don't speak the languages, so can't say.

I too didn't realise the extent of regional variations until this bug came in.

@jamuhl
Copy link
Member

jamuhl commented Oct 9, 2017

rather sure http://www.unicode.org/cldr/charts/26/supplemental/language_plural_rules.html is right...it should be n > 1 for pt-BR

@jamuhl
Copy link
Member

jamuhl commented Oct 10, 2017

made the needed change to i18next and published an update: i18next/i18next@c5113fb

@rankida
Copy link
Contributor Author

rankida commented Oct 10, 2017

Thanks for your hard work.

I guess a similar change will be needed here too?

const ext = plurals.rules[locale.replace('_', '-').split('-')[0]];

@jamuhl
Copy link
Member

jamuhl commented Oct 10, 2017

right...i hope @perrin4869 will do this change in the next days....

@rankida
Copy link
Contributor Author

rankida commented Oct 10, 2017

Thanks again.

If needed I could look at proposing a Pull Request based on your i18next change.

@perrin4869
Copy link
Contributor

@rankida if you send a PR, I can review it on Sunday. It would be highly appreciated

@rankida
Copy link
Contributor Author

rankida commented Oct 12, 2017

No problem @perrin4869 I will give it a go.

One question - @jamuhl's change i18next/i18next@c5113fb moves European Portuguese (pt-PT) to be the special case, rather than Brazilian Portuguese. This seems to agree with the Unicode docs but differs from your commented reference of http://translate.sourceforge.net/wiki/l10n/pluralforms

Would you like me to keep it inline with i18next which means changing the plurals.js definition (and therefore differ from your previous reference) or keep it inline with http://translate.sourceforge.net/wiki/l10n/pluralforms ?

@jamuhl
Copy link
Member

jamuhl commented Oct 12, 2017

@rankida i think / believe / guess unicode docs are wider used...that's why i decided on using their favour ;)

@rankida
Copy link
Contributor Author

rankida commented Oct 12, 2017

Let me know if #68 needs fixedup @perrin4869

@perrin4869
Copy link
Contributor

Merged and published as 5.0.0, please reopen if the problem persists!

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

No branches or pull requests

3 participants