From d88b237f1e95544e192f2bca8512bfccf8bba860 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 28 Feb 2020 14:45:31 +0000 Subject: [PATCH] refactor(localize): add support for TranslationParser diagnostics (#35793) This modifies the internal (but shared with CLI) API for loading/parsing translation files. Now the parsers will return a new `Diagnostics` object along with any translations and locale extracted from the file. It is up to the caller to decide what to do about this, if there are errors it is suggested that an error is thrown, which is what the `TranslationLoader` class does. PR Close #35793 --- .../localize/src/tools/src/diagnostics.ts | 11 +++++ .../localize/src/tools/src/translate/main.ts | 2 +- .../translation_files/translation_loader.ts | 49 +++++++++++++------ .../simple_json_translation_parser.ts | 3 +- .../translation_parsers/translation_parser.ts | 2 + .../xliff1_translation_parser.ts | 4 +- .../xliff2_translation_parser.ts | 4 +- .../xtb_translation_parser.ts | 8 ++- .../src/tools/src/translate/translator.ts | 1 + .../src/tools/test/diagnostics_spec.ts | 48 ++++++++++++++++++ .../translation_loader_spec.ts | 10 ++-- 11 files changed, 115 insertions(+), 27 deletions(-) create mode 100644 packages/localize/src/tools/test/diagnostics_spec.ts diff --git a/packages/localize/src/tools/src/diagnostics.ts b/packages/localize/src/tools/src/diagnostics.ts index 5ba8a52862..1be6a7cccb 100644 --- a/packages/localize/src/tools/src/diagnostics.ts +++ b/packages/localize/src/tools/src/diagnostics.ts @@ -15,4 +15,15 @@ export class Diagnostics { get hasErrors() { return this.messages.some(m => m.type === 'error'); } warn(message: string) { this.messages.push({type: 'warning', message}); } error(message: string) { this.messages.push({type: 'error', message}); } + formatDiagnostics(message: string): string { + const errors = this.messages !.filter(d => d.type === 'error').map(d => ' - ' + d.message); + const warnings = this.messages !.filter(d => d.type === 'warning').map(d => ' - ' + d.message); + if (errors.length) { + message += '\nERRORS:\n' + errors.join('\n'); + } + if (warnings.length) { + message += '\nWARNINGS:\n' + warnings.join('\n'); + } + return message; + } } diff --git a/packages/localize/src/tools/src/translate/main.ts b/packages/localize/src/tools/src/translate/main.ts index 3c2c04233d..bb683f62b6 100644 --- a/packages/localize/src/tools/src/translate/main.ts +++ b/packages/localize/src/tools/src/translate/main.ts @@ -142,7 +142,7 @@ export function translateFiles({sourceRootPath, sourceFilePaths, translationFile [ new Xliff2TranslationParser(), new Xliff1TranslationParser(), - new XtbTranslationParser(diagnostics), + new XtbTranslationParser(), new SimpleJsonTranslationParser(), ], diagnostics); diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_loader.ts b/packages/localize/src/tools/src/translate/translation_files/translation_loader.ts index 33096987b1..10cd4c112e 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_loader.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_loader.ts @@ -14,7 +14,9 @@ import {TranslationParser} from './translation_parsers/translation_parser'; * Use this class to load a collection of translation files from disk. */ export class TranslationLoader { - constructor(private translationParsers: TranslationParser[], private diagnostics: Diagnostics) {} + constructor( + private translationParsers: TranslationParser[], + /** @deprecated */ private diagnostics?: Diagnostics) {} /** * Load and parse the translation files into a collection of `TranslationBundles`. @@ -34,22 +36,37 @@ export class TranslationLoader { return translationFilePaths.map((filePath, index) => { const fileContents = FileUtils.readFile(filePath); for (const translationParser of this.translationParsers) { - if (translationParser.canParse(filePath, fileContents)) { - const providedLocale = translationFileLocales[index]; - const {locale: parsedLocale, translations} = - translationParser.parse(filePath, fileContents); - const locale = providedLocale || parsedLocale; - if (locale === undefined) { - throw new Error( - `The translation file "${filePath}" does not contain a target locale and no explicit locale was provided for this file.`); - } - if (parsedLocale !== undefined && providedLocale !== undefined && - parsedLocale !== providedLocale) { - this.diagnostics.warn( - `The provided locale "${providedLocale}" does not match the target locale "${parsedLocale}" found in the translation file "${filePath}".`); - } - return {locale, translations}; + const result = translationParser.canParse(filePath, fileContents); + if (!result) { + continue; } + + const {locale: parsedLocale, translations, diagnostics} = + translationParser.parse(filePath, fileContents); + if (diagnostics.hasErrors) { + throw new Error(diagnostics.formatDiagnostics( + `The translation file "${filePath}" could not be parsed.`)); + } + + const providedLocale = translationFileLocales[index]; + const locale = providedLocale || parsedLocale; + if (locale === undefined) { + throw new Error( + `The translation file "${filePath}" does not contain a target locale and no explicit locale was provided for this file.`); + } + + if (parsedLocale !== undefined && providedLocale !== undefined && + parsedLocale !== providedLocale) { + diagnostics.warn( + `The provided locale "${providedLocale}" does not match the target locale "${parsedLocale}" found in the translation file "${filePath}".`); + } + + // If we were passed a diagnostics object then copy the messages over to it. + if (this.diagnostics) { + this.diagnostics.messages.push(...diagnostics.messages); + } + + return {locale, translations, diagnostics}; } throw new Error( `There is no "TranslationParser" that can parse this translation file: ${filePath}.`); diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/simple_json_translation_parser.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/simple_json_translation_parser.ts index 0421529e96..e21299215d 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/simple_json_translation_parser.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/simple_json_translation_parser.ts @@ -7,6 +7,7 @@ */ import {ɵMessageId, ɵParsedTranslation, ɵparseTranslation} from '@angular/localize'; import {extname} from 'path'; +import {Diagnostics} from '../../../diagnostics'; import {ParsedTranslationBundle, TranslationParser} from './translation_parser'; /** @@ -32,6 +33,6 @@ export class SimpleJsonTranslationParser implements TranslationParser { const targetMessage = translations[messageId]; parsedTranslations[messageId] = ɵparseTranslation(targetMessage); } - return {locale: parsedLocale, translations: parsedTranslations}; + return {locale: parsedLocale, translations: parsedTranslations, diagnostics: new Diagnostics()}; } } diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts index 802e6dfd7d..126e1c69d9 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parser.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {ɵMessageId, ɵParsedTranslation} from '@angular/localize/private'; +import {Diagnostics} from '../../../diagnostics'; /** * An object that holds translations that have been parsed from a translation file. @@ -13,6 +14,7 @@ import {ɵMessageId, ɵParsedTranslation} from '@angular/localize/private'; export interface ParsedTranslationBundle { locale: string|undefined; translations: Record<ɵMessageId, ɵParsedTranslation>; + diagnostics: Diagnostics; } /** diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff1_translation_parser.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff1_translation_parser.ts index 1301d5ecda..3efa5ea209 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff1_translation_parser.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff1_translation_parser.ts @@ -9,6 +9,7 @@ import {Element, Node, XmlParser, visitAll} from '@angular/compiler'; import {ɵMessageId, ɵParsedTranslation} from '@angular/localize'; import {extname} from 'path'; +import {Diagnostics} from '../../../diagnostics'; import {BaseVisitor} from '../base_visitor'; import {MessageSerializer} from '../message_serialization/message_serializer'; import {TargetMessageRenderer} from '../message_serialization/target_message_renderer'; @@ -55,7 +56,8 @@ class XliffFileElementVisitor extends BaseVisitor { if (element.name === 'file') { this.bundle = { locale: getAttribute(element, 'target-language'), - translations: XliffTranslationVisitor.extractTranslations(element) + translations: XliffTranslationVisitor.extractTranslations(element), + diagnostics: new Diagnostics(), }; } else { return visitAll(this, element.children); diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff2_translation_parser.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff2_translation_parser.ts index 3530150ea8..12f35626a8 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff2_translation_parser.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xliff2_translation_parser.ts @@ -9,6 +9,7 @@ import {Element, Node, XmlParser, visitAll} from '@angular/compiler'; import {ɵMessageId, ɵParsedTranslation} from '@angular/localize'; import {extname} from 'path'; +import {Diagnostics} from '../../../diagnostics'; import {BaseVisitor} from '../base_visitor'; import {MessageSerializer} from '../message_serialization/message_serializer'; import {TargetMessageRenderer} from '../message_serialization/target_message_renderer'; @@ -61,7 +62,8 @@ class Xliff2TranslationBundleVisitor extends BaseVisitor { } else if (element.name === 'file') { this.bundle = { locale: parsedLocale, - translations: Xliff2TranslationVisitor.extractTranslations(element) + translations: Xliff2TranslationVisitor.extractTranslations(element), + diagnostics: new Diagnostics(), }; } else { return visitAll(this, element.children, {parsedLocale}); diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xtb_translation_parser.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xtb_translation_parser.ts index a0358f3491..a2f122f812 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xtb_translation_parser.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/xtb_translation_parser.ts @@ -24,7 +24,7 @@ import {getAttrOrThrow, parseInnerRange} from './translation_utils'; * A translation parser that can load XB files. */ export class XtbTranslationParser implements TranslationParser { - constructor(private diagnostics: Diagnostics) {} + constructor(private diagnostics: Diagnostics = new Diagnostics()) {} canParse(filePath: string, contents: string): boolean { const extension = extname(filePath); @@ -61,7 +61,11 @@ class XtbVisitor extends BaseVisitor { element.sourceSpan, ' elements can not be nested'); } const langAttr = element.attrs.find((attr) => attr.name === 'lang'); - bundle = {locale: langAttr && langAttr.value, translations: {}}; + bundle = { + locale: langAttr && langAttr.value, + translations: {}, + diagnostics: this.diagnostics + }; visitAll(this, element.children, bundle); return bundle; diff --git a/packages/localize/src/tools/src/translate/translator.ts b/packages/localize/src/tools/src/translate/translator.ts index 1cda8b75d0..74ccebada5 100644 --- a/packages/localize/src/tools/src/translate/translator.ts +++ b/packages/localize/src/tools/src/translate/translator.ts @@ -19,6 +19,7 @@ import {OutputPathFn} from './output_path'; export interface TranslationBundle { locale: string; translations: Record<ɵMessageId, ɵParsedTranslation>; + diagnostics?: Diagnostics; } /** diff --git a/packages/localize/src/tools/test/diagnostics_spec.ts b/packages/localize/src/tools/test/diagnostics_spec.ts new file mode 100644 index 0000000000..c2c32a8644 --- /dev/null +++ b/packages/localize/src/tools/test/diagnostics_spec.ts @@ -0,0 +1,48 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {Diagnostics} from '../src/diagnostics'; + +describe('Diagnostics', () => { + describe('formatDiagnostics', () => { + it('should just return the message passed in if there are no errors nor warnings', () => { + const diagnostics = new Diagnostics(); + expect(diagnostics.formatDiagnostics('This is a message')).toEqual('This is a message'); + }); + + it('should return a string with all the errors listed', () => { + const diagnostics = new Diagnostics(); + diagnostics.error('Error 1'); + diagnostics.error('Error 2'); + diagnostics.error('Error 3'); + expect(diagnostics.formatDiagnostics('This is a message')) + .toEqual('This is a message\nERRORS:\n - Error 1\n - Error 2\n - Error 3'); + }); + + it('should return a string with all the warnings listed', () => { + const diagnostics = new Diagnostics(); + diagnostics.warn('Warning 1'); + diagnostics.warn('Warning 2'); + diagnostics.warn('Warning 3'); + expect(diagnostics.formatDiagnostics('This is a message')) + .toEqual('This is a message\nWARNINGS:\n - Warning 1\n - Warning 2\n - Warning 3'); + }); + + it('should return a string with all the errors and warnings listed', () => { + const diagnostics = new Diagnostics(); + diagnostics.warn('Warning 1'); + diagnostics.warn('Warning 2'); + diagnostics.error('Error 1'); + diagnostics.error('Error 2'); + diagnostics.warn('Warning 3'); + diagnostics.error('Error 3'); + expect(diagnostics.formatDiagnostics('This is a message')) + .toEqual( + 'This is a message\nERRORS:\n - Error 1\n - Error 2\n - Error 3\nWARNINGS:\n - Warning 1\n - Warning 2\n - Warning 3'); + }); + }); +}); diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_loader_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_loader_spec.ts index 0fd0a5c299..0d9995d557 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_loader_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_loader_spec.ts @@ -58,8 +58,8 @@ describe('TranslationLoader', () => { const result = loader.loadBundles(['/src/locale/messages.en.xlf', '/src/locale/messages.fr.xlf'], []); expect(result).toEqual([ - {locale: 'pl', translations}, - {locale: 'pl', translations}, + {locale: 'pl', translations, diagnostics: new Diagnostics()}, + {locale: 'pl', translations, diagnostics: new Diagnostics()}, ]); }); @@ -71,8 +71,8 @@ describe('TranslationLoader', () => { const result = loader.loadBundles( ['/src/locale/messages.en.xlf', '/src/locale/messages.fr.xlf'], ['en', 'fr']); expect(result).toEqual([ - {locale: 'en', translations}, - {locale: 'fr', translations}, + {locale: 'en', translations, diagnostics: new Diagnostics()}, + {locale: 'fr', translations, diagnostics: new Diagnostics()}, ]); }); @@ -127,6 +127,6 @@ class MockTranslationParser implements TranslationParser { parse(filePath: string, fileContents: string) { this.log.push(`parse(${filePath}, ${fileContents})`); - return {locale: this._locale, translations: this._translations}; + return {locale: this._locale, translations: this._translations, diagnostics: new Diagnostics()}; } } \ No newline at end of file