Skip to content

fix(common): use correct ICU plural rules for all locales#16990

Merged
vicb merged 1 commit into
angular:masterfrom
ocombe:fix-plurals-script
May 30, 2017
Merged

fix(common): use correct ICU plural rules for all locales#16990
vicb merged 1 commit into
angular:masterfrom
ocombe:fix-plurals-script

Conversation

@ocombe

@ocombe ocombe commented May 24, 2017

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

[x] Bugfix

What is the current behavior?
Some locales with empty plural rules were using the wrong plural rule instead of the default one because of a bug in the extraction script.

What is the new behavior?
The extraction script has been fixed and the localization file updated accordingly. All the locales that were miscategorized have been removed so that they now use the default rule.
I've also updated the script based on the changes that have been done to the localization script (even if the code is supposed to be generated, some people have changed the content without updating the script).

Does this PR introduce a breaking change?

[x] No

@ocombe ocombe added area: i18n Issues related to localization and internationalization action: review The PR is still awaiting reviews from at least one requested reviewer labels May 24, 2017
@ocombe ocombe requested a review from vicb May 24, 2017 15:16
Comment thread scripts/cldr/gen_plural_rules.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why experimental & export ?

Comment thread scripts/cldr/gen_plural_rules.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove 'experimental'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ignore my previous comment (this is generated code)

@vicb

vicb commented May 24, 2017

Copy link
Copy Markdown
Contributor

needs tests !

@ocombe ocombe force-pushed the fix-plurals-script branch 2 times, most recently from d85f8fb to 71f414f Compare May 24, 2017 15:45

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ocombe I'm wondering if we should return one instead, could you please do some research to figure out ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Already checked the other day, it's always "other" when there is no rule in the spec

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"for a locale with no rule" ?

@ocombe ocombe force-pushed the fix-plurals-script branch 3 times, most recently from 400f566 to 4323b0a Compare May 24, 2017 17:31
Comment thread packages/common/src/localization.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be l 398 ?

Locales with no rules were using the wrong plural rule instead of the default. The extraction script has been fixed and the localization file updated accordingly.
@ocombe ocombe force-pushed the fix-plurals-script branch from 4323b0a to 9e76676 Compare May 24, 2017 17:44
@vicb vicb removed the action: review The PR is still awaiting reviews from at least one requested reviewer label May 24, 2017
@vicb vicb added the action: merge The PR is ready for merge by the caretaker label May 24, 2017
@vicb vicb merged commit 535d9da into angular:master May 30, 2017
@ocombe ocombe deleted the fix-plurals-script branch June 8, 2017 08:19
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
…angular#16990)

Locales with no rules were using the wrong plural rule instead of the default.
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
…angular#16990)

Locales with no rules were using the wrong plural rule instead of the default.
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: i18n Issues related to localization and internationalization cla: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants