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

fix: full cldr #33682

Closed
wants to merge 2 commits into from
Closed

fix: full cldr #33682

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2019

Blocked on #33699

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Fixes #33681

What is the new behavior?

Enables all the previous locales, plus new ones as per cldr-36

Does this PR introduce a breaking change?

No, but the PR whose behavior it fixes, does

Some locales were removed, some were renamed.

@ghost ghost requested review from a team as code owners November 8, 2019 09:01
@petebacondarwin petebacondarwin added area: common Issues related to APIs in the @angular/common package area: i18n action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: bug/fix labels Nov 8, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 8, 2019
@petebacondarwin petebacondarwin added the area: build & ci Related the build and CI infrastructure of the project label Nov 8, 2019
@petebacondarwin
Copy link
Member

Nice fast work @doom777 - I was just looking at that configuration too :-)

@petebacondarwin
Copy link
Member

petebacondarwin commented Nov 8, 2019

Note that you must remove node_modules and reinstall for the cldr-data post install hook to run and for it to pick up the new flag from the package.json.

@ghost ghost mentioned this pull request Nov 8, 2019
14 tasks
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this fix!

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

something fishy is going on here. we should dig into this more before merging the PR

@@ -19,17 +19,21 @@ export default [
'agq', [['a.g', 'a.k'], u, u], u,
[
['n', 'k', 'g', 't', 'u', 'g', 'd'], ['nts', 'kpa', 'ghɔ', 'tɔm', 'ume', 'ghɨ', 'dzk'],
['tsuʔntsɨ', 'tsuʔukpà', 'tsuʔughɔe', 'tsuʔutɔ̀mlò', 'tsuʔumè', 'tsuʔughɨ̂m', 'tsuʔndzɨkɔʔɔ'],
[
Copy link
Contributor

Choose a reason for hiding this comment

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

it's odd that the formatting is now different but the content is the same

Copy link
Member

Choose a reason for hiding this comment

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

Again, I have found that clang will refuse to format some files on my machine. I don't know why.

@@ -57,7 +57,8 @@ export default [
'RUR': [],
'SEK': [],
'THB': ['฿'],
'TWD': ['NT$']
'TWD': ['NT$'],
'XXX': []
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case there seems to be a data change. is that expected?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

yes, data change is expected, because it is part of update from cldr33 to cldr36, where some of the files were not yet upgraded.

]
],
[['е.ә.', 'ј.е.'], u, ['ерамыздан әввәл', 'јени ера']], 1, [6, 0],
['dd.MM.yy', 'd MMM y', 'd MMMM y', 'd MMMM y, EEEE'],
['HH:mm', 'HH:mm:ss', 'HH:mm:ss z', 'HH:mm:ss zzzz'], ['{1} {0}', u, u, u],
[',', '.', ';', '%', '+', '-', 'E', '×', '‰', '∞', 'NaN', ':'],
['#,##0.###', '#,##0%', '¤ #,##0.00', '#E0'], '₼', 'AZN',
['#,##0.###', '#,##0%', '#,##0.00 ¤', '#E0'], '₼', 'AZN',
Copy link
Contributor

Choose a reason for hiding this comment

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

another suspicious data change

Copy link
Member

Choose a reason for hiding this comment

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

this could well have been a fix from when we upgraded to 36 but that didn't show up previously if this is one of the files that was missing from that previous PR.

packages/common/locales/az-Latn.ts Outdated Show resolved Hide resolved
packages/common/locales/closure-locale.ts Outdated Show resolved Hide resolved
packages/common/locales/closure-locale.ts Outdated Show resolved Hide resolved

const u = undefined;

export default [];
Copy link
Contributor

Choose a reason for hiding this comment

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

are these supposed to be empty?

Copy link
Member

Choose a reason for hiding this comment

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

Many of the extra files are empty, which is why we just concatenated them together in the "global" versions.

@josephperrott
Copy link
Member

@doom777 I sorted out the cause of the Unicode replacement characters showing up, and it will be sorted out in #33699. Once we get that in, you will need to rebase this PR on it and then we can merge this in since we definitely want to have the full cldr information set.

@josephperrott
Copy link
Member

@doom777 With #33699 now merged, you should be able to rebase and rerun the script to create the files without any � characters.

Ephraim added 2 commits November 11, 2019 23:52
switching to cldr-data package resulted in
loss of some locales, since by default only core locales are loaded.
This PR adds a flag to tell cldr-data to use full locale coverage

fixes: #33681
@ghost
Copy link
Author

ghost commented Nov 11, 2019

Rebasing done, @IgorMinar

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM after the rebase

@IgorMinar
Copy link
Contributor

Thanks!

@josephperrott josephperrott added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 11, 2019
@kara kara closed this in 472641c Nov 11, 2019
kara pushed a commit that referenced this pull request Nov 11, 2019
kara pushed a commit that referenced this pull request Nov 11, 2019
switching to cldr-data package resulted in
loss of some locales, since by default only core locales are loaded.
This PR adds a flag to tell cldr-data to use full locale coverage

fixes: #33681

PR Close #33682
kara pushed a commit that referenced this pull request Nov 11, 2019
@ghost ghost deleted the fix/fullcldr branch November 12, 2019 19:48
@angular-automatic-lock-bot
Copy link

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 Dec 13, 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: build & ci Related the build and CI infrastructure of the project area: common Issues related to APIs in the @angular/common package area: i18n cla: yes target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Locale Regression: 150 locales purged
4 participants