call Translation.Get & GetC directly

[chrisvaughn]

instead of calling Translation.GetN with a default of plural=1

• add correct Arabic plural rules to test

Before creating your Pull Request...

• New Pull Requests should include a good description of what's being merged.
• Ideally, all Pull Requests are preceded by a discussion initiated in an Issue on this repository.
• For bug fixes is mandatory to have tests that cover and fail when the bug is present and will pass after this Pull Request.
• For changes and improvements, new tests have to be provided to cover the new features.

Is this a fix, improvement or something else?

Fix (https://github.com/leonelquinteros/gotext/issues/39) ...

What does this change implement/fix?

Adding the correct Pluralform rules in the existing Arabic test case caused it to fail in a way that's consistent with https://github.com/leonelquinteros/gotext/issues/39 The code changes provided allowed the test to pass ...

I have ...

• [x] answered the 2 questions above,
• [x] discussed this change in an issue,
• [x] included tests to cover this changes.

[leonelquinteros]

@chrisvaughn Thanks!

I was trying to avoid code duplication by calling the GetN functions with n=1 for singulars, but I see now that's not safe on some languages to catch malformed or incomplete translation strings. I'm still not sure if this is something we need to solve, or if that's for the translator to provide a complete translation file.

Please note that the main issue here is that you're using a complex plural formula string in the header, but then not setting any plural string in the PO file. So, this case contains the following conditions:

• Plurals formula present + Plural translation string missing

Can you extend the test cases to cover the following in the arabic tests?:

• Plurals formula missing + Plural translation string missing
• Plurals formula present + Plural translation string present without translations (so, only the msgid_plural, but not msgstr[n])
• Plurals formula present + Plural translation string present and complete (matching translations with all available plural forms)

With these cases, we should cover all malformed translation files and also the combinations of translation retrieval -> result.

Also, shouldn't we also replicate that in the package level functions (see gotext.go). Can you work on that and add the corresponding tests to these?

For reference when writing tests, take into account that when the translation is missing, the package always tries to return the original untranslated string. When requesting for Plural translations and the translation is missing, the package tries to return the original untranslated plural string if N != 1, otherwise it will return the original untranslated singular string.

[chrisvaughn]

Thanks for the feedback! I understand the desire to reduce code duplicate when so much of it is the same. I've looked at other ways but I couldn't come up with anything clean. I can add those test cases and look at adding this to the gotext package level functions. Maybe once we've validated the code duplication version is working as we want and we have tests we can look at refactors to reduce duplication.

I think I might be missing something and I want to make sure I understand where you are coming from so I can contribute my best to the project. I don't think my case is "incomplete or malformed translation strings". The files I'm testing with are generated by our translation tool, Crowdin, and seem to match the standard. I have several strings and some use plurals and some that don't. I don't think it's standard to provide plural for non-plural strings. Do you disagree with that?

[chrisvaughn]

@leonelquinteros I've added the updates to testing and made the same in the gotext package level functions. Let me know if you have any other feedback. Thanks

[leonelquinteros]

Thank you very much @chrisvaughn !!!

Sorry for the delay reviewing this, I got sink in work this week.

I agree con the code duplication issue, it's not important if it doesn't works well. Proper behavior is more important than code hygiene.

You're also right on having valid translation files, I didn't expressed myself well, I meant that you were trying to get a translation that wasn't even defined, but as you already figured out, it should behave differently.

So, thanks again for your contribution. It's really valuable to have you contributing as it seems you'll be covering a LOT of use cases for this package that otherwise wouldn't ever be handled.

Feel free to keep sending PRs with new tests and other findings you may have, I'll be happy to include test cases that may be useful for you to ensure forward compatibility so you can also benefit from your own contributions to the package.

[chrisvaughn]

Thanks @leonelquinteros. I appreciate you working with me on this.