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

Added be, bs, sh, sr, uk languages #99

Closed
wants to merge 1 commit into from
Closed

Added be, bs, sh, sr, uk languages #99

wants to merge 1 commit into from

Conversation

viatsko
Copy link
Contributor

@viatsko viatsko commented Feb 3, 2018

According to
http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html#lv-comp,
Belarussian and Ukrainian languages have the same plural form rules,
so it makes sense to add them to the russian plural forms group.

Tests for be, ru, uk languages plural forms added to document this behavior.

Closes #94.

 - add generic Bosnian (`bs`) and Serbian (`sr`) codes

According to http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html, Belarussian, Ukrainian, Bosnian, Serbian and Serbian-Croatian languages have the same plural form rules, so it makes sense to add them to the russian plural forms group.
@viatsko viatsko changed the title Added be, uk languages to ru plural forms Added be, uk languages, fix for lt language Feb 3, 2018
@viatsko viatsko changed the title Added be, uk languages, fix for lt language Added be, bs, sh, sr, uk languages, fix for lt language Feb 3, 2018
@viatsko
Copy link
Contributor Author

viatsko commented Feb 3, 2018

For Turkish it's trickier as in most of the cases number will remain in a singular form, but if a translation string will be used for, per example, 2 sentences, then there's no chance to set the right form for the 2nd sentence, good example is here http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html#tr

As putting tr in a correct group wouldn't affect any existing translation tags (as they're singular anyway), I'd suggest keeping the third commit.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This seems like multiple conceptually different changes all in the same PR, which makes it harder to review:

  • tr moves from "chinese" to "german"
  • adding new russian langs
  • moving lithuanian from "russian" to its own plural rules

Would you mind peeling out 2 of those 3 changes into separate PRs?

@viatsko
Copy link
Contributor Author

viatsko commented Feb 4, 2018

Sure, makes sense! Hold on :)

@viatsko viatsko closed this Feb 4, 2018
@ljharb
Copy link
Collaborator

ljharb commented Feb 4, 2018

Thanks :-) actually it'd be great if you could reuse this PR for one of the changes :-) Force pushing is always OK!

@ljharb ljharb reopened this Feb 4, 2018
@viatsko
Copy link
Contributor Author

viatsko commented Feb 4, 2018

This one should be good to go, although I can add more tests for the whole family

@viatsko viatsko changed the title Added be, bs, sh, sr, uk languages, fix for lt language Added be, bs, sh, sr, uk languages Feb 4, 2018
test/index.js Outdated
@@ -180,6 +180,69 @@ describe('locale-specific pluralization rules', function () {
expect(polyglot.t('n_votes', 11)).to.equal('11 صوت');
expect(polyglot.t('n_votes', 102)).to.equal('102 صوت');
});

it('pluralizes in Belarussian', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it "belarussian" or "belorussian"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, it's "Belarusian" :)

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

It looks like you added the following languages:

  • Belarusian (with tests)
  • Bosnian (no tests)
  • Serbo-Croatian (no tests)
  • Serbian (no tests)
  • Ukrainian (with tests)

Do you think it would be useful to add tests for those 3?

@viatsko
Copy link
Contributor Author

viatsko commented Feb 5, 2018

Think it makes sense to have tests for all the languages to document the behavior, but tests for these for sure would make a lot of sense to prevent unintended changes.

Added!

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

It'd be great if you could rebase this down to a single commit; if not, I'm happy to do it prior to merging.

@viatsko
Copy link
Contributor Author

viatsko commented Feb 6, 2018

Squashed

@thedotedge
Copy link

Any plans to merge this one?

Copy link

@yzimet yzimet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ljharb
Copy link
Collaborator

ljharb commented Nov 29, 2023

@viatsko unfortunately you deleted your repo, so i can't land this; could you please open a new PR?

@ljharb ljharb marked this pull request as draft November 29, 2023 19:35
@ljharb ljharb closed this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants