fix(ivy): i18n - support setting locales for each translation file #33381

petebacondarwin · 2019-10-24T18:58:58Z

Previously the target locale of a translation file had to be extracted
from the contents of the translation file.

Now an array of locales can be provided via the translationFileLocales
option that overrides any target locale extracted from the file.
This allows us to support translation files that do not have a target
locale specified in their contents.

// FW-1644
Fixes #33323

packages/localize/src/tools/src/translate/translation_files/translation_file_loader.ts

IgorMinar

the rest lgtm

...localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts

...ools/src/translate/translation_files/translation_parsers/xliff1/xliff1_translation_parser.ts

...c/tools/test/translate/translation_files/translation_parsers/simple_json/simple_json_spec.ts

petebacondarwin · 2019-10-25T10:11:29Z

After @clydin's feedback I completely redesigned this PR - so all the files have changed since the last reviews (cc @IgorMinar).

@clydin please note that the return type from TranslationParser.parse() has changed. I think you are using this directly in the CLI integration?? So this will be a breaking change for this integration.

PTAL!!

IgorMinar

A few more suggestions.

IgorMinar · 2019-10-25T15:03:31Z

packages/localize/src/tools/src/translate/translation_files/translation_loader.ts

+          if (parsedLocale !== undefined && providedLocale !== undefined &&
+              parsedLocale !== providedLocale) {
+            this.diagnostics.warn(
+                `The provided locale "${providedLocale}" and the target locale "${parsedLocale}" do not match for translation file "${filePath}".`);


I don't think that this is easy to understand. How about "specified locale XX doesn't match locale YY found in the translation file"

IgorMinar · 2019-10-25T15:17:29Z

packages/localize/src/tools/test/translate/translation_files/translation_loader_spec.ts

+      const loader = new TranslationLoader([parser], diagnostics);
+      expect(() => loader.loadBundles(['/src/locale/messages.en.xlf'], []))
+          .toThrowError(
+              'No target locale provided nor found in the translation file "/src/locale/messages.en.xlf"');


Maybe add "please provide --target-locale argument"?

The only problem with this is that it might be called via the Angular CLI which does not have this option...

How about: "consider explicitly providing a target-locale for this file"?

clydin · 2019-10-25T17:44:47Z

...localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts

+*/
+export interface ParsedTranslationBundle {
+  parsedLocale: string|undefined;
+  parsedTranslations: Record<ɵMessageId, ɵParsedTranslation>;


If these names are kept unchanged, no code changes would be needed on the CLI side.

This is where it is used in the CLI (for context): https://github.com/angular/angular-cli/blob/395c97912800af435021432e708cbdfab28783fe/packages/angular_devkit/build_angular/src/utils/load-translations.ts

Only the translations property appears to be important...

I'll switch them back...

petebacondarwin · 2019-10-25T21:13:24Z

As I am a code-owner of this, and having approval from @IgorMinar, I think that this can be merged, right?

AndrewKushnir

LGTM, just left a couple comments. Thanks Pete.

...c/tools/test/translate/translation_files/translation_parsers/simple_json/simple_json_spec.ts

AndrewKushnir · 2019-10-25T21:18:01Z

packages/localize/src/tools/src/translate/main.ts


  diagnostics.messages.forEach(m => console.warn(`${m.type}: ${m.message}`));
  process.exit(diagnostics.hasErrors ? 1 : 0);
 }

 export interface TranslateFilesOptions {
+  /**


Thanks for adding docs 👍

Previously the target locale of a translation file had to be extracted from the contents of the translation file. Therefore it was an error if the translation file did not provide a target locale. Now an array of locales can be provided via the `translationFileLocales` option that overrides any target locale extracted from the file. This allows us to support translation files that do not have a target locale specified in their contents. // FW-1644 Fixes angular#33323

The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales.

…#33381) The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales. PR Close #33381

…ngular#33381) Previously the target locale of a translation file had to be extracted from the contents of the translation file. Therefore it was an error if the translation file did not provide a target locale. Now an array of locales can be provided via the `translationFileLocales` option that overrides any target locale extracted from the file. This allows us to support translation files that do not have a target locale specified in their contents. // FW-1644 Fixes angular#33323 PR Close angular#33381

…angular#33381) The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales. PR Close angular#33381

…ngular#33381) Previously the target locale of a translation file had to be extracted from the contents of the translation file. Therefore it was an error if the translation file did not provide a target locale. Now an array of locales can be provided via the `translationFileLocales` option that overrides any target locale extracted from the file. This allows us to support translation files that do not have a target locale specified in their contents. // FW-1644 Fixes angular#33323 PR Close angular#33381

…angular#33381) The `localize-translate` command line tool can now accept an array of target locales to support the case where translation files do not contain them. Specify this array via the `--target-locales` option. NOTE to early adopters: in order to support this, the original `-t` option for the binary has changed from being a glob pattern to an array of paths, which will have matching indices to any provided target-locales. PR Close angular#33381

angular-automatic-lock-bot · 2019-11-28T16:09:44Z

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

petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n target: major This PR is targeted for the next major release comp: ivy labels Oct 24, 2019

petebacondarwin requested a review from a team as a code owner October 24, 2019 18:58

ngbot bot modified the milestone: needsTriage Oct 24, 2019

googlebot added the cla: yes label Oct 24, 2019

petebacondarwin mentioned this pull request Oct 24, 2019

test: enable existing aot i18n E2E test for ivy angular/angular-cli#15913

Closed

clydin reviewed Oct 24, 2019

View reviewed changes

packages/localize/src/tools/src/translate/translation_files/translation_file_loader.ts Outdated Show resolved Hide resolved

petebacondarwin 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 fixed by Ivy labels Oct 24, 2019

IgorMinar approved these changes Oct 24, 2019

View reviewed changes

petebacondarwin force-pushed the i18n-missing-target-language branch from 9af849e to 1850ad9 Compare October 25, 2019 10:05

petebacondarwin requested a review from a team as a code owner October 25, 2019 10:05

petebacondarwin force-pushed the i18n-missing-target-language branch from 1850ad9 to 28fd971 Compare October 25, 2019 10:06

petebacondarwin force-pushed the i18n-missing-target-language branch from 28fd971 to 646302c Compare October 25, 2019 10:59

IgorMinar reviewed Oct 25, 2019

View reviewed changes

clydin reviewed Oct 25, 2019

View reviewed changes

petebacondarwin force-pushed the i18n-missing-target-language branch 4 times, most recently from d742d1c to 529d94c Compare October 25, 2019 21:10

AndrewKushnir approved these changes Oct 25, 2019

View reviewed changes

petebacondarwin added 2 commits October 25, 2019 22:23

petebacondarwin force-pushed the i18n-missing-target-language branch from 529d94c to c7a3767 Compare October 25, 2019 21:23

clydin approved these changes Oct 25, 2019

View reviewed changes

petebacondarwin mentioned this pull request Oct 28, 2019

i18n: support xmb formatted translation files in compile time inlining #33444

Closed

AndrewKushnir closed this in 62b2840 Oct 28, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): i18n - support setting locales for each translation file #33381

fix(ivy): i18n - support setting locales for each translation file #33381

petebacondarwin commented Oct 24, 2019

IgorMinar left a comment

petebacondarwin commented Oct 25, 2019

IgorMinar left a comment

IgorMinar Oct 25, 2019

IgorMinar Oct 25, 2019

petebacondarwin Oct 25, 2019

petebacondarwin Oct 25, 2019

clydin Oct 25, 2019

petebacondarwin Oct 25, 2019

petebacondarwin Oct 25, 2019

petebacondarwin commented Oct 25, 2019

AndrewKushnir left a comment

AndrewKushnir Oct 25, 2019

angular-automatic-lock-bot bot commented Nov 28, 2019

fix(ivy): i18n - support setting locales for each translation file #33381

fix(ivy): i18n - support setting locales for each translation file #33381

Conversation

petebacondarwin commented Oct 24, 2019

IgorMinar left a comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 25, 2019

IgorMinar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 25, 2019

AndrewKushnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Nov 28, 2019