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

[localize] Add <x equiv-text> XLIFF placeholder style and use by default #2275

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented Oct 29, 2021

See #2271 for more context.

XLIFF is the XML format we use to represent extracted templates/strings that need translation. XLIFF specifies multiple ways of encoding placeholders (for representing HTML markup and dynamic expressions). The differences according to the spec are a bit confusing:

  • <ph>: "Placeholder - The element is used to delimit a sequence of native stand-alone codes in the translation unit."
  • <x>: "Generic placeholder - The element is used to replace any code of the original document."

Previously we were using <ph>, because the spec seemed to match what we need, and I was primed by XLB (Google's very similar format) which also uses <ph> tags for this purpose. However, I found that in practice translation tools seem to have much better support for the XLIFF <x> tag (I have tested crowdin, phrase, and lokalise). Additionally, <x> is the approach used by Angular (see https://angular.io/guide/i18n-example), so it is likely that translation tools/services have been already tested with Angular style message extraction.

This is possibly breaking because it changes the default from <ph> to <x>, but upgrading both source and translated messages to <x> will happen automatically when the user next runs lit-localize extract. We retain the ability to use <ph> tags by setting a new config file setting.

Fixes #2271

cc @fetherston fyi

@changeset-bot
Copy link

changeset-bot bot commented Oct 29, 2021

🦋 Changeset detected

Latest commit: df1fb5b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-cla google-cla bot added the cla: yes label Oct 29, 2021
@aomarks aomarks changed the title Add <x equiv-text> XLIFF placeholder style and use by default [localize] Add <x equiv-text> XLIFF placeholder style and use by default Oct 29, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2021

📊 Tachometer Benchmark Results

Summary

  • lit-element-list
  • lit-html-kitchen-sink
  • lit-html-repeat
  • lit-html-template-heavy
  • reactive-element-list

Results

tachometer-reporter-action v2 for Benchmarks

@fetherston
Copy link

Looks like the message ids remain the same right?

@aomarks
Copy link
Member Author

aomarks commented Oct 30, 2021

Looks like the message ids remain the same right?

Yep, this will have no effect on message IDs. If you upgrade and do nothing else, the next time you run extract the placeholders will change to the new syntax, both in the <source> and <target> elements. If that confuses some part of the localization pipeline for some reason, you can switch back to the old style with "placeholderStyle": "ph".

@@ -118,7 +127,9 @@ export class XliffFormatter implements Formatter {
!phText ||
phText.nodeType !== doc.TEXT_NODE
) {
throw new KnownError(`Expected <ph> to have exactly one text node`);
throw new KnownError(
`Expected <${child.nodeName}> to have exactly one text node`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to punt, but this seems like a good candidate for a diagnostic

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not in a TypeScript context here -- do you mean something that displays similar to how a TypeScript diagnostic displays with a text snippet and underline etc.?

@@ -0,0 +1,12 @@
/**
* @license
* Copyright 2020 Google LLC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here and elsewhere: 2021

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a rename, so it's copyright date shouldn't change (right?). And the other new files are also clones.

@@ -0,0 +1 @@
git won't track an empty directory
Copy link
Collaborator

Choose a reason for hiding this comment

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

(is this necessary? looks like there are files in this dir)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@aomarks aomarks merged commit 97f4a3f into main Nov 8, 2021
@aomarks aomarks deleted the localize-ph-x branch November 8, 2021 16:31
@github-actions github-actions bot mentioned this pull request Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[localize] Prefer <x> to <ph> in generated XLIFF files
3 participants