feat(compiler) : i18n explicit ID by mallex-actee · Pull Request #13272 · angular/angular

mallex-actee · 2016-12-06T14:06:48Z

Please check if the PR fulfills these 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)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
See [i18n] enable control over translation message ID

What is the new behavior?
Developer can now use specific ID in translations by adding a new "parameter" in the i18n attribute :

<p i18n="meaning|description@id">foo</p>
<!-- i18n: meaning1|desc1@@id1 -->bar<!-- /i18n -->

will produce for XMB :

<msg id="id" meaning="meaning" description="description">foo</msg>
<msg id="id1" meaning="meaning1" description="description1">foo</msg>

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

mallex-actee · 2016-12-06T14:08:11Z

@vicb I created new PR, because I'm not very good at rebasing. Now you have a clean PR.

vicb · 2016-12-06T17:24:11Z

modules/@angular/compiler/src/i18n/extractor_merger.ts

what do you think or renaming i18nInfo to msgInfo / msgMeta ?

@vicb OK for msgMeta.

vicb · 2016-12-06T17:24:36Z

modules/@angular/compiler/src/i18n/extractor_merger.ts

_parseMessageMeta would look better, WDYT ?

vicb · 2016-12-06T17:25:20Z

modules/@angular/compiler/src/i18n/extractor_merger.ts

please return an obj now, would be clerarer

vicb · 2016-12-06T17:27:10Z

modules/@angular/compiler/src/i18n/extractor_merger.ts

Please look for @@ first.

split left part on | for m|d,

right part is id

add const MEANING_SEPARATOR = "|" and const ID_SEPARATOR = "@@"

vicb · 2016-12-06T17:28:02Z

modules/@angular/compiler/test/i18n/digest_spec.ts

add trailing "," here please - and maybe move id first (this is the important part)

vicb · 2016-12-06T17:30:49Z

Thanks !

I have added a last round of comments.

To rebase, update your master branch from upstream (or origin whatever it is name) and do a git rebase master -i in your feature branch.
That's pretty easy, do not hesitate to ask if you need help.

Also please remove the WS before the colon in feat(..) : ..., thanks

vicb · 2016-12-06T18:52:30Z

Also we discussed about an issue/PR on the doc repo, any progress on that front ?

mallex-actee · 2016-12-07T10:56:02Z

@vicb see the angular.io PR => docs(compiler): add doc + examples for i18n explicit ID option

vicb · 2016-12-07T18:36:07Z

modules/@angular/compiler/src/i18n/extractor_merger.ts

move the const at file level

vicb · 2016-12-07T18:40:44Z

modules/@angular/compiler/src/i18n/extractor_merger.ts

simplify, something like:

const idIndex = i18n.indexOf(ID_SEPARATOR); const [mAndD, id] = idIndex == -1 ? [i18n, ''] : [i18n.slice(0, idIndex), i18n.slice(idIndex + 2)]; const descIndex = i18n.indexOf('|'); const [meaning, description] = descIndex == -1 ? ['', mAndD] : [mAndD.slice(0, descIndex), i18n.slice(descIndex + 1)]; return {meaning, description, id};

vicb · 2016-12-07T18:41:56Z

modules/@angular/compiler/test/i18n/extractor_merger_spec.ts

nit: "id only"

vicb · 2016-12-07T18:44:04Z

Thanks, this PRs looks good. Only a couple more changes before this can be merged.

vicb · 2016-12-09T05:51:12Z

Thanks! it should get in tomorrow.

kibbled · 2016-12-21T20:47:34Z

Would this feature allow us to set IDs in our HTML/XLF files and avoid having to maintain IDs changing?

angular-automatic-lock-bot · 2019-09-10T12:21:23Z

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

googlebot added the cla: yes label Dec 6, 2016

mallex-actee mentioned this pull request Dec 6, 2016

[i18n] enable control over translation message ID #12941

Closed

mallex-actee force-pushed the i18n-explicit-id branch from 0350625 to bde38c5 Compare December 6, 2016 14:56

vicb reviewed Dec 6, 2016

View reviewed changes

modules/@angular/compiler/src/i18n/extractor_merger.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Dec 6, 2016

Choose a reason for hiding this comment

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

_parseMessageMeta would look better, WDYT ?

vicb reviewed Dec 6, 2016

View reviewed changes

modules/@angular/compiler/src/i18n/extractor_merger.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Dec 6, 2016

Choose a reason for hiding this comment

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

please return an obj now, would be clerarer

vicb reviewed Dec 6, 2016

View reviewed changes

mallex-actee force-pushed the i18n-explicit-id branch from bde38c5 to 8f8af59 Compare December 7, 2016 08:51

mallex-actee force-pushed the i18n-explicit-id branch from 8f8af59 to 129d9c7 Compare December 7, 2016 10:58

vicb reviewed Dec 7, 2016

View reviewed changes

modules/@angular/compiler/src/i18n/extractor_merger.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Dec 7, 2016

Choose a reason for hiding this comment

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

move the const at file level

vicb reviewed Dec 7, 2016

View reviewed changes

modules/@angular/compiler/test/i18n/extractor_merger_spec.ts Outdated

Copy link
Copy Markdown

Contributor

vicb Dec 7, 2016

Choose a reason for hiding this comment

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

nit: "id only"

mallex-actee added 3 commits December 8, 2016 09:54

feat(compiler): add id property to i18nMessage

9d5fbf7

feat(compiler): digest methods return i18nMessage id if sets

03c874f

test(compiler): test i18n explicit id

b868ce2

mallex-actee force-pushed the i18n-explicit-id branch from 129d9c7 to b868ce2 Compare December 8, 2016 08:54

vicb added action: merge The PR is ready for merge by the caretaker 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 9, 2016

vicb removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 9, 2016

vicb closed this in 56c361f Dec 9, 2016

vicb mentioned this pull request Jan 4, 2017

Allow specifying an i18n ID rather than generating it from the content #13781

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

Conversation

mallex-actee commented Dec 6, 2016

Uh oh!

mallex-actee commented Dec 6, 2016

Uh oh!

vicb Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

mallex-actee Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

vicb Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

vicb Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

vicb Dec 6, 2016 • 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!

vicb Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

vicb commented Dec 6, 2016

Uh oh!

vicb commented Dec 6, 2016

Uh oh!

mallex-actee commented Dec 7, 2016

Uh oh!

vicb Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

vicb Dec 7, 2016 • 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!

vicb Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

vicb commented Dec 7, 2016

Uh oh!

vicb commented Dec 9, 2016

Uh oh!

kibbled commented Dec 21, 2016

Uh oh!

angular-automatic-lock-bot bot commented Sep 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vicb Dec 6, 2016 •

edited

Loading

vicb Dec 7, 2016 •

edited

Loading