Skip to content

Conversation

@wenqi73
Copy link
Contributor

@wenqi73 wenqi73 commented Dec 31, 2020

@angular/localize supports extracting as json file, but CLI does not support this.

@google-cla
Copy link

google-cla bot commented Dec 31, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Dec 31, 2020
@wenqi73
Copy link
Contributor Author

wenqi73 commented Dec 31, 2020

@googlebot I fixed it.

@google-cla
Copy link

google-cla bot commented Dec 31, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: yes and removed cla: no labels Dec 31, 2020
@wenqi73 wenqi73 changed the title chore(@angular-devkit/build-angular): ng extract-i18n support json fix(@angular-devkit/build-angular): ng extract-i18n support json Dec 31, 2020
@petebacondarwin
Copy link
Contributor

@wenqi73
Copy link
Contributor Author

wenqi73 commented Jan 5, 2021

@petebacondarwin What do you mean by "implement merging translations"? Could you tell me the details?

@petebacondarwin
Copy link
Contributor

@wenqi73 - there are two main processes in i18n: extraction and merging (sometimes called translation or inlining). The extraction process involves parsing the code for $localize calls and rendering translation files (e.g. XLIFF) containing all the i18n messages. The merging process involves parsing the code for $localize calls and replacing those calls with translated messages, where the translated messages are read in from translation files (e.g. XLIFF).

When supporting a particular translation file format, we need to provide a serializer (used when extracting) and a parser (used when merging).

@wenqi73
Copy link
Contributor Author

wenqi73 commented Jan 5, 2021

@petebacondarwin It already has the JSON parser in cli,

so just need to add serializer? And now in my project I also use JSON files as translation files with localize-extract, it works fine!

@petebacondarwin
Copy link
Contributor

Gadzooks! You are right! I didn't see that. Nice. OK so you are good just to add the serializer. Sorry for the noise.

@wenqi73 wenqi73 changed the title fix(@angular-devkit/build-angular): ng extract-i18n support json feat(@angular-devkit/build-angular): ng extract-i18n support json and arb Jan 5, 2021
@wenqi73
Copy link
Contributor Author

wenqi73 commented Jan 5, 2021

@petebacondarwin I have added arb format, please have a look😄

@petebacondarwin petebacondarwin added the target: minor This PR is targeted for the next minor release label Jan 5, 2021
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

Can you also add an E2E test file for each of the formats?
You can use the following test file as a base (and the new tests can be placed in the same directory):
https://github.com/angular/angular-cli/blob/cfb368408b532e488acaa1a5a921240f0dc7dfad/tests/legacy-cli/e2e/tests/i18n/ivy-localize-dl-xmb.ts

Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

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

Code changes look good.
Can you re-structure the commits so that the PR contains two feature commits: one for JSON and one for ARB (with the tests for each included in their respective commits)?

With that adjustment this should be good to be merged.
Thank you for the contribution.

@wenqi73 wenqi73 force-pushed the add-json branch 2 times, most recently from 1243758 to c9c8642 Compare January 7, 2021 03:06
@wenqi73
Copy link
Contributor Author

wenqi73 commented Jan 7, 2021

@clydin Commits have been readjusted.

Copy link
Member

@clydin clydin 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 again for the contribution.

@clydin clydin added the action: merge The PR is ready for merge by the caretaker label Jan 8, 2021
@clydin clydin removed their assignment Jan 8, 2021
@alan-agius4 alan-agius4 merged commit ab72f8d into angular:master Jan 8, 2021
@wenqi73 wenqi73 deleted the add-json branch January 9, 2021 02:58
@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 Feb 9, 2021
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 target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants