Add writing direction to locale files · Pull Request #33556 · angular/angular

ghost · 2019-11-03T13:37:55Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

Locale files do not contain writing direction

What is the new behavior?

Locale files will contain writing direction

Does this PR introduce a breaking change?

Yes
No

Other information

Look at first commit for actual changes. Second commit is just generated locale files

ghost · 2019-11-03T14:34:02Z

If you're wondering the purpose of this PR, it is to eventually support angular/angular-cli#16047
Currently, creating an RTL locale build does not make the page RTL

petebacondarwin

Thanks for this PR - generally looks good. A couple of items to look at.
We should also consider whether this should land before or after #33523 as they clearly conflict with each other.

I think also that since we are well into the RC now, and this has a public API change, then this will probably go into 9.1.0

packages/common/src/i18n/locale_data_api.ts

tools/gulp-tasks/cldr/extract.js

packages/common/src/i18n/locale_data_api.ts

ghost · 2019-11-04T11:44:44Z

@petebacondarwin do you suggest I rebase this change on top of #33523?

petebacondarwin · 2019-11-04T11:49:48Z

Let's hold off for 24 hours while we work out what release this could go in and what to do about that other PR. Cheers

petebacondarwin · 2019-11-04T12:08:11Z

I've marked this as target: master only as I think it might have to go into 9.1.x.
@IgorMinar do you have a view?

josephperrott

LGTM, for dev-infra

ghost · 2019-11-05T17:47:15Z

I noticed that #33523 was merged in. I guess I'll rebase this PR on top of master.

petebacondarwin · 2019-11-06T08:38:48Z

Thanks @doom777 - sorry I didn't get back to you earlier.

petebacondarwin · 2019-11-06T08:43:54Z

This is now competing with #33584 since they will also conflict :-/

ghost · 2019-11-06T08:57:35Z

@petebacondarwin true, but my PR was here first ;-)
Also, this PR is further ahead of the other one, since we got more approvals, and ~~are merged with master~~.
Also, if you really insist, I can rebase this PR on top of that one also.

Edit: they are also merged with #33523

petebacondarwin · 2019-11-06T09:01:37Z

The question is really whether this can go into master & patch (e.g. 9.0.0) or only master (e.g. 9.1.0), since it is arguably a new public API and we are now in RC for 9.0.0. cc @IgorMinar / @kara ?

ghost · 2019-11-06T15:43:08Z

@petebacondarwin BTW, why are these locale files checked into source control? Why not generate them during the build process?

petebacondarwin · 2019-11-06T16:12:22Z

That is a good question. It has been like this for many years.
I think it is nice not to have to generate them locally when developing that Angular framework.

That being said we should look into ways to automate the update of these to some Bazel rules...

josephperrott · 2019-11-06T16:20:13Z

This is actually an outstanding task for the @angular/dev-infra-framework team. To determine if continuing to store them in the repository is the best option or if they should be generated at build time. And with whichever method, how to best do that going forward.

AndrewKushnir · 2019-11-06T18:09:25Z

Adding @IgorMinar to reviewers list, since there are changes to public API that need to be reviewed/approved. @IgorMinar, could you please have a look at the first commit (where the API changes are)? Thank you.

ghost · 2019-11-08T09:35:27Z

blocked by: #33682

ghost · 2019-11-21T20:36:41Z

Hey, so we are in action:merge right now, so what else needs to be done?

matsko · 2019-11-21T21:54:19Z

Sorry @doom777, the presubmits need to run again before merging. ETA 1-2 hours.

matsko · 2019-11-21T21:56:11Z

presubmit = https://test.corp.google.com/ui#id=OCL:281825168:BASE:281825201:1574373305055:9a1b9da8

ivy-presubmit = https://tap.corp.google.com/ui#id=OCL:281825168:BASE:281825333:1574373342060:bf55f5c0

IgorMinar

Hi there, I rereviewed the PR again and noticed that getLocaleDirection is not reexported from @angular/core which means that the function is not available via the public api surface, rendering this feature inaccessible to developers (outside of our own unit tests).

Can you please update the PR with proper reexports, which will also trigger the need to update the public api guard (the CI failure will instruct you what command to run to do that).

While this PR is large, it should not conflict with other PRs, so we shouldn't rush to get it in because of a fear that it will bit rot. It's more important that we merge it in in a functional state.

ghost · 2019-11-22T12:49:42Z

@IgorMinar Thanks for that note. I have updated the code. Please review it.
Of course you're correct that it's more important that the PR is functional, then just merged.

ghost · 2019-11-25T20:36:43Z

Hey, @IgorMinar @petebacondarwin can this PR move forward?

ghost · 2019-11-28T17:15:57Z

Hey guys, just another ping on this PR.

petebacondarwin · 2019-11-28T17:37:04Z

I'm afraid it is Thanksgiving in US - so it is unlikely anything will move on this before Tuesday.

ghost · 2019-12-03T13:10:33Z

Hey, everyone. It's Tuesday here. Just pinging.

IgorMinar

Lgtm now. Thank you for making all the follow up changes.

IgorMinar · 2019-12-03T17:06:47Z

merge-assistance: global approval & caretaker can you please run presubmit?

mhevery · 2019-12-03T18:10:00Z

presubmit

PR Close #33556

… data (#34240)" This reverts commit a02bde7.

angular-automatic-lock-bot · 2020-01-03T16:05:37Z

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.}

ghost requested review from a team November 3, 2019 13:37

googlebot added the cla: yes label Nov 3, 2019

ghost self-requested a review November 3, 2019 14:22

petebacondarwin suggested changes Nov 4, 2019

View reviewed changes

petebacondarwin approved these changes Nov 4, 2019

View reviewed changes

petebacondarwin added area: i18n Issues related to localization and internationalization target: major This PR is targeted for the next major release feature Label used to distinguish feature request from other issues labels Nov 4, 2019

ngbot bot modified the milestone: needsTriage Nov 4, 2019

petebacondarwin self-assigned this Nov 4, 2019

josephperrott approved these changes Nov 4, 2019

View reviewed changes

petebacondarwin added action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 6, 2019

AndrewKushnir requested a review from IgorMinar November 6, 2019 18:08

mhevery 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 18, 2019

IgorMinar suggested changes Nov 22, 2019

View reviewed changes

IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Nov 22, 2019

ghost self-requested a review November 22, 2019 10:16

Feliks Khantsis added 2 commits November 22, 2019 15:07

feat: add direction property to locale files

4ac5ac5

feat: update the locale files

e69890d

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 3, 2019

IgorMinar approved these changes Dec 3, 2019

View reviewed changes

mhevery closed this in 3c24384 Dec 3, 2019

mhevery pushed a commit that referenced this pull request Dec 3, 2019

feat: update the locale files (#33556)

15d3e74

PR Close #33556

mhevery referenced this pull request Dec 11, 2019

Revert "fix(common): update closure locales to include directionality…

1183075

… data (#34240)" This reverts commit a02bde7.

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 3, 2020

Conversation

ghost commented Nov 3, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

ghost commented Nov 3, 2019

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Nov 4, 2019

Uh oh!

petebacondarwin commented Nov 4, 2019

Uh oh!

petebacondarwin commented Nov 4, 2019

Uh oh!

josephperrott left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 5, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Nov 6, 2019

Uh oh!

petebacondarwin commented Nov 6, 2019

Uh oh!

ghost commented Nov 6, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Nov 6, 2019

Uh oh!

ghost commented Nov 6, 2019

Uh oh!

petebacondarwin commented Nov 6, 2019

Uh oh!

josephperrott commented Nov 6, 2019

Uh oh!

AndrewKushnir commented Nov 6, 2019

Uh oh!

ghost commented Nov 8, 2019

Uh oh!

ghost commented Nov 21, 2019

Uh oh!

matsko commented Nov 21, 2019

Uh oh!

matsko commented Nov 21, 2019

Uh oh!

IgorMinar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 22, 2019

Uh oh!

ghost commented Nov 25, 2019

Uh oh!

ghost commented Nov 28, 2019

Uh oh!

petebacondarwin commented Nov 28, 2019

Uh oh!

ghost commented Dec 3, 2019

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhevery commented Dec 3, 2019

Uh oh!

angular-automatic-lock-bot bot commented Jan 3, 2020

Uh oh!

ghost commented Nov 3, 2019 •

edited by ghost

Loading

ghost commented Nov 5, 2019 •

edited by ghost

Loading

ghost commented Nov 6, 2019 •

edited by ghost

Loading

IgorMinar left a comment •

edited

Loading

IgorMinar commented Dec 3, 2019 •

edited

Loading