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

feat(compiler): add "original" placeholder value on extracted XMB #25079

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Goodwine
Contributor

Goodwine commented Jul 24, 2018

Update XMB placeholders(<ph>) to include the original value on top of an
example. Placeholders can by definition have one example(<ex>) tag and a
text node. The text node is used by TC as the "original" value from the
placeholder, while the example should represent a dummy value.
For example: <ph name="PET"><ex>Gopher</ex>{{ petName }}</ph>.
This change makes sure that we have the original text, but it DOES NOT
make sure that the example is correct. The example has the same wrong
behavior of showing the interpolation text rather than a useful example.

No breaking changes, but tools that depend on the previous behavior and
don't consider the full XMB definition may fail to parse the XMB.
Fixes b/72565847

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[x] 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
[x] Other... Please describe: XMB extraction

What is the current behavior?

A message like <foo>Name: {{yourName}}</foo> would generate this xmb message:

<msg id=123>Name: <ph name="INTERPOLATION"><ex>{{yourName}}</ex></ph></msg>

Issue Number: http://b/72565847

What is the new behavior?

A message like <foo>Name: {{yourName}}</foo> would generate this xmb message:

<msg id=123>Name: <ph name="INTERPOLATION"><ex>{{yourName}}</ex>{{yourName}}</ph></msg>

The <ph> PCDATA (text node) is used by TC for some validations explained further in the internal bug.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Tools that depend on the current (wrong) implementation may fail if they don't take the #PCDATA definition into account.

Other information

Ideally the example should be something like <ex>Carlos</ex>, but fixing the example is out of scope because there is no way to provide one.

@googlebot googlebot added the cla: yes label Jul 24, 2018

@Goodwine

This comment has been minimized.

Contributor

Goodwine commented Jul 24, 2018

Note: These bazel tests were already failing

//packages/compiler-cli/test/compliance:compliance
//packages/core/test/bundling/hello_world:symbol_test
//packages/core/test/bundling/hello_world:test
//packages/core/test/bundling/todo:symbol_test
//packages/core/test/bundling/todo:test

@@ -43,6 +43,7 @@ jasmine_node_test(
],
deps = [
":extract_i18n_lib",
"//packages/common:npm_package",

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

Could you explain why this change is required ? Thx

This comment has been minimized.

@Goodwine

Goodwine Jul 25, 2018

Contributor

CommonModule is needed to process ICU syntax, otherwise angular complains that it doesn't know how to handle ICU. The module is imported in a generated "module" test file similar to @NgModule, but CommonModule is in a different package.

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

Why not importing //packages/common instead ?

This comment has been minimized.

@Goodwine

Goodwine Jul 25, 2018

Contributor

I copied it from another rule, when changing it to //packages/common, bazel fails the test //packages/compiler-cli/test:extract_i18n because "error TS2307: Cannot find module '@angular/common'."

@vicb

This comment has been minimized.

Contributor

vicb commented Jul 25, 2018

@Goodwine Thanks for the PR. I'll do my best to review it by Thursday.

@@ -115,30 +115,36 @@ class _Visitor implements i18n.Visitor {
}
visitTagPlaceholder(ph: i18n.TagPlaceholder, context?: any): xml.Node[] {
const startEx = new xml.Tag(_EXEMPLE_TAG, {}, [new xml.Text(`<${ph.tag}>`)]);
const startTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name: ph.startName}, [startEx]);
const originalStartTag = new xml.Text(`<${ph.tag}>`);

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

nit: rename to startTagAsText or something along those line

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

Please also add a comment explaining that the TC uses this extra text node (for all placeholder types).

if (ph.isVoid) {
// void tags have no children nor closing tags
return [startTagPh];
}
const closeEx = new xml.Tag(_EXEMPLE_TAG, {}, [new xml.Text(`</${ph.tag}>`)]);
const closeTagPh = new xml.Tag(_PLACEHOLDER_TAG, {name: ph.closeName}, [closeEx]);
const originalCloseTag = new xml.Text(`</${ph.tag}>`);

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

nit: closeTagAsText

]);
return [new xml.Tag(_PLACEHOLDER_TAG, {name: ph.name}, [exTag])];
const originalIcu = new xml.Text(
`{${ph.value.expression}, ${ph.value.type}, ${Object.keys(ph.value.cases).map((value: string) => value + ' {...}').join(' ')}}`);

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

nit: would you mind extracting the ${} to local consts ?
(I know this has not been introduced by you but it would help readability anyway)

This comment has been minimized.

@Goodwine

Goodwine Jul 25, 2018

Contributor

will do

<msg id="703464324060964421"><source>file.ts:22,24</source>
<ph name="ICU"><ex>{sex, select, male {...} female {...} other {...}}</ex></ph>
<ph name="ICU"><ex>male, female, other</ex>{sex, select, male {...} female {...} other {...}}</ph>

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

is changing the <ex> a requirement from the TC

This comment has been minimized.

@Goodwine

Goodwine Jul 25, 2018

Contributor

it's not, <ex> is used by translators to know what kind of values to expect, I figured it would make sense to only list the options instead of the icu's text representation

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

I would prefer not to change this one in this PR, Coud you please revert this change.

<msg id="3780349238193953556"><source>file.ts:9</source><source>file.ts:10</source><ph name="START_ITALIC_TEXT"><ex>&lt;i&gt;</ex></ph>with placeholders<ph name="CLOSE_ITALIC_TEXT"><ex>&lt;/i&gt;</ex></ph></msg>
<msg id="5415448997399451992"><source>file.ts:11</source><ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex></ph>with <ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex></ph>nested<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex></ph> placeholders<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex></ph></msg>
<msg id="3780349238193953556"><source>file.ts:9</source><source>file.ts:10</source><ph name="START_ITALIC_TEXT"><ex>&lt;i&gt;</ex>&lt;i&gt;</ph>with placeholders<ph name="CLOSE_ITALIC_TEXT"><ex>&lt;/i&gt;</ex>&lt;/i&gt;</ph></msg>
<msg id="5415448997399451992"><source>file.ts:11</source><ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex>&lt;div&gt;</ph>with <ph name="START_TAG_DIV"><ex>&lt;div&gt;</ex>&lt;div&gt;</ph>nested<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex>&lt;/div&gt;</ph> placeholders<ph name="CLOSE_TAG_DIV"><ex>&lt;/div&gt;</ex>&lt;/div&gt;</ph></msg>

This comment has been minimized.

@vicb

vicb Jul 25, 2018

Contributor

<div> looks wrong here - is it github transforming encoded entities ?

This comment has been minimized.

@Goodwine

Goodwine Jul 25, 2018

Contributor

This is the XML library when doing new xml.Text(`<${ph.tag}>`);, it's WAI

@jermowery

This comment has been minimized.

jermowery commented Jul 25, 2018

+100,000,000 this will eliminate an enormous annoyance

@vicb

vicb approved these changes Jul 25, 2018

Thank you very much for this PR.
Could you please address the minor comments ?

@Goodwine

This comment has been minimized.

Contributor

Goodwine commented Jul 25, 2018

Thanks for the review, I've addressed the comments, PTAL

@vicb

This comment has been minimized.

Contributor

vicb commented Jul 25, 2018

Could you please revert the <ex> change - everything else is great.
Thanks !

edit:

  • CircleCi issues should be solved after a rebase,
  • I have restarted Travis - very flaky ATM
feat(compiler): add "original" placeholder value on extracted XMB
Update XMB placeholders(<ph>) to include the original value on top of an
example. Placeholders can by definition have one example(<ex>) tag and a
text node. The text node is used by TC as the "original" value from the
placeholder, while the example should represent a dummy value.
For example: <ph name="PET"><ex>Gopher</ex>{{ petName }}</ph>.
This change makes sure that we have the original text, but it *DOES NOT*
make sure that the example is correct. The example has the same wrong
behavior of showing the interpolation text rather than a useful
example.

No breaking changes, but tools that depend on the previous behavior and
don't consider the full XMB definition may fail to parse the XMB.
Fixes b/72565847
@Goodwine

This comment has been minimized.

Contributor

Goodwine commented Jul 25, 2018

PTAL
Alright, I reverted the ICU <ex> change. I also rebased to upstream/master so hopefully circleci won't fail now. The 5 test targets I mentioned earlier are still failing tho.

@vicb

This comment has been minimized.

@vicb

This comment has been minimized.

Contributor

vicb commented Jul 30, 2018

merge-assistance: missing pullapprove but the change is only about a symbol that has been renamed (to fix a typo). Please merge. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment