From 32f33f095f472e09c83b038f68918d42b941c681 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 2 Sep 2020 12:38:22 +0100 Subject: [PATCH] fix(localize): render context of translation file parse errors (#38673) Previously the position of the error in a translation file when parsing it was not displayed. Just the error message. Now the position (line and column) and some context is displayed along with the error messages. Fixes #38377 PR Close #38673 --- .../message_serializer.ts | 2 +- .../serialize_translation_message.ts | 32 +++++++++++++++ .../translation_parse_error.ts | 18 ++++----- .../translation_parsers/translation_utils.ts | 25 ++++++++---- .../xliff1_translation_parser.ts | 31 +++++--------- .../xliff2_translation_parser.ts | 35 ++++++---------- .../xtb_translation_parser.ts | 40 ++++++++----------- .../xliff1_translation_parser_spec.ts | 28 +++++++------ .../xliff2_translation_parser_spec.ts | 18 +++++---- .../xtb_translation_parser_spec.ts | 26 ++++++++---- 10 files changed, 140 insertions(+), 115 deletions(-) create mode 100644 packages/localize/src/tools/src/translate/translation_files/translation_parsers/serialize_translation_message.ts diff --git a/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts b/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts index 995e6fbebd975..af0fd9c3a75e9 100644 --- a/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts +++ b/packages/localize/src/tools/src/translate/translation_files/message_serialization/message_serializer.ts @@ -13,7 +13,7 @@ import {getAttribute, getAttrOrThrow} from '../translation_parsers/translation_u import {MessageRenderer} from './message_renderer'; -interface MessageSerializerConfig { +export interface MessageSerializerConfig { inlineElements: string[]; placeholder?: {elementName: string; nameAttribute: string; bodyAttribute?: string;}; placeholderContainer?: {elementName: string; startAttribute: string; endAttribute: string;}; diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/serialize_translation_message.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/serialize_translation_message.ts new file mode 100644 index 0000000000000..df247b1801fd2 --- /dev/null +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/serialize_translation_message.ts @@ -0,0 +1,32 @@ +/** + * @license + * Copyright Google LLC 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 {Element, ParseError} from '@angular/compiler'; +import {ɵParsedTranslation} from '@angular/localize'; + +import {MessageSerializer, MessageSerializerConfig} from '../message_serialization/message_serializer'; +import {TargetMessageRenderer} from '../message_serialization/target_message_renderer'; + +import {parseInnerRange} from './translation_utils'; + +/** + * Serialize the given `element` into a parsed translation using the given `serializer`. + */ +export function serializeTranslationMessage(element: Element, config: MessageSerializerConfig): { + translation: ɵParsedTranslation|null, + parseErrors: ParseError[], + serializeErrors: ParseError[] +} { + const {rootNodes, errors: parseErrors} = parseInnerRange(element); + try { + const serializer = new MessageSerializer(new TargetMessageRenderer(), config); + const translation = serializer.serialize(rootNodes); + return {translation, parseErrors, serializeErrors: []}; + } catch (e) { + return {translation: null, parseErrors, serializeErrors: [e]}; + } +} diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts index 5074335dabfdc..7fc6adfc67d22 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_parse_error.ts @@ -14,17 +14,15 @@ export class TranslationParseError extends Error { constructor( public span: ParseSourceSpan, public msg: string, public level: ParseErrorLevel = ParseErrorLevel.ERROR) { - super(msg); - } - - contextualMessage(): string { - const ctx = this.span.start.getContext(100, 3); - return ctx ? `${this.msg} ("${ctx.before}[${ParseErrorLevel[this.level]} ->]${ctx.after}")` : - this.msg; + super(contextualMessage(span, msg, level)); } +} - toString(): string { - const details = this.span.details ? `, ${this.span.details}` : ''; - return `${this.contextualMessage()}: ${this.span.start}${details}`; +function contextualMessage(span: ParseSourceSpan, msg: string, level: ParseErrorLevel): string { + const ctx = span.start.getContext(100, 2); + msg += `\nAt ${span.start}${span.details ? `, ${span.details}` : ''}:\n`; + if (ctx) { + msg += `...${ctx.before}[${ParseErrorLevel[level]} ->]${ctx.after}...\n`; } + return msg; } diff --git a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts index e45738740fc13..f44cd87e93919 100644 --- a/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts +++ b/packages/localize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts @@ -5,10 +5,12 @@ * 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 {Element, LexerRange, Node, ParseError, ParseErrorLevel, ParseSourceSpan, XmlParser} from '@angular/compiler'; +import {Element, LexerRange, Node, ParseError, ParseErrorLevel, ParseSourceSpan, ParseTreeResult, XmlParser} from '@angular/compiler'; + import {Diagnostics} from '../../../diagnostics'; + import {TranslationParseError} from './translation_parse_error'; -import {ParseAnalysis} from './translation_parser'; +import {ParseAnalysis, ParsedTranslationBundle} from './translation_parser'; export function getAttrOrThrow(element: Element, attrName: string): string { const attrValue = getAttribute(element, attrName); @@ -30,17 +32,15 @@ export function getAttribute(element: Element, attrName: string): string|undefin * This would be equivalent to parsing the `innerHTML` string of an HTML document. * * @param element The element whose inner range we want to parse. - * @returns a collection of XML `Node` objects that were parsed from the element's contents. + * @returns a collection of XML `Node` objects and any errors that were parsed from the element's + * contents. */ -export function parseInnerRange(element: Element): Node[] { +export function parseInnerRange(element: Element): ParseTreeResult { const xmlParser = new XmlParser(); const xml = xmlParser.parse( element.sourceSpan.start.file.content, element.sourceSpan.start.file.url, {tokenizeExpansionForms: true, range: getInnerRange(element)}); - if (xml.errors.length) { - throw xml.errors.map(e => new TranslationParseError(e.span, e.msg).toString()).join('\n'); - } - return xml.rootNodes; + return xml; } /** @@ -155,3 +155,12 @@ export function addParseError(diagnostics: Diagnostics, parseError: ParseError): diagnostics.warn(parseError.toString()); } } + +/** + * Add the provided `errors` to the `bundle` diagnostics. + */ +export function addErrorsToBundle(bundle: ParsedTranslationBundle, errors: ParseError[]): void { + for (const error of errors) { + addParseError(bundle.diagnostics, error); + } +} 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 0f0636b10e1ed..7ac869632f370 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 @@ -6,15 +6,13 @@ * found in the LICENSE file at https://angular.io/license */ import {Element, ParseErrorLevel, visitAll} from '@angular/compiler'; -import {ɵParsedTranslation} from '@angular/localize'; import {Diagnostics} from '../../../diagnostics'; import {BaseVisitor} from '../base_visitor'; -import {MessageSerializer} from '../message_serialization/message_serializer'; -import {TargetMessageRenderer} from '../message_serialization/target_message_renderer'; +import {serializeTranslationMessage} from './serialize_translation_message'; import {ParseAnalysis, ParsedTranslationBundle, TranslationParser} from './translation_parser'; -import {addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, parseInnerRange, XmlTranslationParserHint} from './translation_utils'; +import {addErrorsToBundle, addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, XmlTranslationParserHint} from './translation_utils'; /** * A translation parser that can load XLIFF 1.2 files. @@ -150,23 +148,14 @@ class XliffTranslationVisitor extends BaseVisitor { return; } - try { - bundle.translations[id] = serializeTargetMessage(targetMessage); - } catch (e) { - // Capture any errors from serialize the target message - if (e.span && e.msg && e.level) { - addParseDiagnostic(bundle.diagnostics, e.span, e.msg, e.level); - } else { - throw e; - } + const {translation, parseErrors, serializeErrors} = serializeTranslationMessage(targetMessage, { + inlineElements: ['g', 'bx', 'ex', 'bpt', 'ept', 'ph', 'it', 'mrk'], + placeholder: {elementName: 'x', nameAttribute: 'id'} + }); + if (translation !== null) { + bundle.translations[id] = translation; } + addErrorsToBundle(bundle, parseErrors); + addErrorsToBundle(bundle, serializeErrors); } } - -function serializeTargetMessage(source: Element): ɵParsedTranslation { - const serializer = new MessageSerializer(new TargetMessageRenderer(), { - inlineElements: ['g', 'bx', 'ex', 'bpt', 'ept', 'ph', 'it', 'mrk'], - placeholder: {elementName: 'x', nameAttribute: 'id'} - }); - return serializer.serialize(parseInnerRange(source)); -} 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 5304665cde804..622ab2d26d1bc 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 @@ -6,15 +6,13 @@ * found in the LICENSE file at https://angular.io/license */ import {Element, Node, ParseErrorLevel, visitAll} from '@angular/compiler'; -import {ɵParsedTranslation} from '@angular/localize'; import {Diagnostics} from '../../../diagnostics'; import {BaseVisitor} from '../base_visitor'; -import {MessageSerializer} from '../message_serialization/message_serializer'; -import {TargetMessageRenderer} from '../message_serialization/target_message_renderer'; +import {serializeTranslationMessage} from './serialize_translation_message'; import {ParseAnalysis, ParsedTranslationBundle, TranslationParser} from './translation_parser'; -import {addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, parseInnerRange, XmlTranslationParserHint} from './translation_utils'; +import {addErrorsToBundle, addParseDiagnostic, addParseError, canParseXml, getAttribute, isNamedElement, XmlTranslationParserHint} from './translation_utils'; /** * A translation parser that can load translations from XLIFF 2 files. @@ -141,29 +139,20 @@ class Xliff2TranslationVisitor extends BaseVisitor { return; } - try { - bundle.translations[unit] = serializeTargetMessage(targetMessage); - } catch (e) { - // Capture any errors from serialize the target message - if (e.span && e.msg && e.level) { - addParseDiagnostic(bundle.diagnostics, e.span, e.msg, e.level); - } else { - throw e; - } + const {translation, parseErrors, serializeErrors} = serializeTranslationMessage(targetMessage, { + inlineElements: ['cp', 'sc', 'ec', 'mrk', 'sm', 'em'], + placeholder: {elementName: 'ph', nameAttribute: 'equiv', bodyAttribute: 'disp'}, + placeholderContainer: + {elementName: 'pc', startAttribute: 'equivStart', endAttribute: 'equivEnd'} + }); + if (translation !== null) { + bundle.translations[unit] = translation; } + addErrorsToBundle(bundle, parseErrors); + addErrorsToBundle(bundle, serializeErrors); } } -function serializeTargetMessage(source: Element): ɵParsedTranslation { - const serializer = new MessageSerializer(new TargetMessageRenderer(), { - inlineElements: ['cp', 'sc', 'ec', 'mrk', 'sm', 'em'], - placeholder: {elementName: 'ph', nameAttribute: 'equiv', bodyAttribute: 'disp'}, - placeholderContainer: - {elementName: 'pc', startAttribute: 'equivStart', endAttribute: 'equivEnd'} - }); - return serializer.serialize(parseInnerRange(source)); -} - function isFileElement(node: Node): node is Element { return node instanceof Element && node.name === 'file'; } 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 e20260896eb8c..d1b8bca839658 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 @@ -5,17 +5,15 @@ * 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 {Element, ParseErrorLevel, visitAll} from '@angular/compiler'; -import {ɵParsedTranslation} from '@angular/localize'; +import {Element, ParseError, ParseErrorLevel, visitAll} from '@angular/compiler'; 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'; +import {serializeTranslationMessage} from './serialize_translation_message'; import {ParseAnalysis, ParsedTranslationBundle, TranslationParser} from './translation_parser'; -import {addParseDiagnostic, addParseError, canParseXml, getAttribute, parseInnerRange, XmlTranslationParserHint} from './translation_utils'; +import {addErrorsToBundle, addParseDiagnostic, addParseError, canParseXml, getAttribute, XmlTranslationParserHint} from './translation_utils'; /** @@ -103,20 +101,17 @@ class XtbVisitor extends BaseVisitor { return; } - try { - bundle.translations[id] = serializeTargetMessage(element); - } catch (error) { - if (typeof error === 'string') { - bundle.diagnostics.warn( - `Could not parse message with id "${ - id}" - perhaps it has an unrecognised ICU format?\n` + - error); - } else if (error.span && error.msg && error.level) { - addParseDiagnostic(bundle.diagnostics, error.span, error.msg, error.level); - } else { - throw error; - } + const {translation, parseErrors, serializeErrors} = serializeTranslationMessage( + element, {inlineElements: [], placeholder: {elementName: 'ph', nameAttribute: 'name'}}); + if (parseErrors.length) { + // We only want to warn (not error) if there were problems parsing the translation for + // XTB formatted files. See https://github.com/angular/angular/issues/14046. + bundle.diagnostics.warn(computeParseWarning(id, parseErrors)); + } else if (translation !== null) { + // Only store the translation if there were no parse errors + bundle.translations[id] = translation; } + addErrorsToBundle(bundle, serializeErrors); break; default: @@ -127,9 +122,8 @@ class XtbVisitor extends BaseVisitor { } } -function serializeTargetMessage(source: Element): ɵParsedTranslation { - const serializer = new MessageSerializer( - new TargetMessageRenderer(), - {inlineElements: [], placeholder: {elementName: 'ph', nameAttribute: 'name'}}); - return serializer.serialize(parseInnerRange(source)); +function computeParseWarning(id: string, errors: ParseError[]): string { + const msg = errors.map(e => e.toString()).join('\n'); + return `Could not parse message with id "${id}" - perhaps it has an unrecognised ICU format?\n` + + msg; } diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts index c69b839433def..9f475e54e8f69 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff1_translation_parser_spec.ts @@ -621,7 +621,7 @@ describe('Xliff1TranslationParser', () => { }); describe('[structure errors]', () => { - it('should throw when a trans-unit has no translation', () => { + it('should fail when a trans-unit has no translation', () => { const XLIFF = [ ``, ``, @@ -646,7 +646,7 @@ describe('Xliff1TranslationParser', () => { }); - it('should throw when a trans-unit has no id attribute', () => { + it('should fail when a trans-unit has no id attribute', () => { const XLIFF = [ ``, ``, @@ -671,7 +671,7 @@ describe('Xliff1TranslationParser', () => { ].join('\n')); }); - it('should throw on duplicate trans-unit id', () => { + it('should fail on duplicate trans-unit id', () => { const XLIFF = [ ``, ``, @@ -703,7 +703,7 @@ describe('Xliff1TranslationParser', () => { }); describe('[message errors]', () => { - it('should throw on unknown message tags', () => { + it('should fail on unknown message tags', () => { const XLIFF = [ ``, ``, @@ -719,17 +719,18 @@ describe('Xliff1TranslationParser', () => { ].join('\n'); expectToFail('/some/file.xlf', XLIFF, /Invalid element found in message/, [ - `Invalid element found in message. ("`, - ` `, + `Error: Invalid element found in message.`, + `At /some/file.xlf@6:16:`, + `...`, ` `, ` [ERROR ->]msg should contain only ph tags`, ` `, - ` `, - `"): /some/file.xlf@6:16`, + `...`, + ``, ].join('\n')); }); - it('should throw when a placeholder misses an id attribute', () => { + it('should fail when a placeholder misses an id attribute', () => { const XLIFF = [ ``, ``, @@ -745,13 +746,14 @@ describe('Xliff1TranslationParser', () => { ].join('\n'); expectToFail('/some/file.xlf', XLIFF, /required "id" attribute/gi, [ - `Missing required "id" attribute: ("`, - ` `, + `Error: Missing required "id" attribute:`, + `At /some/file.xlf@6:16:`, + `...`, ` `, ` [ERROR ->]`, ` `, - ` `, - `"): /some/file.xlf@6:16`, + `...`, + ``, ].join('\n')); }); }); diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts index 9bcae2900bcb3..d9b412c8bc486 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xliff2_translation_parser_spec.ts @@ -650,13 +650,14 @@ describe( ].join('\n'); expectToFail('/some/file.xlf', XLIFF, /Invalid element found in message/, [ - `Invalid element found in message. ("`, - ` `, + `Error: Invalid element found in message.`, + `At /some/file.xlf@6:16:`, + `...`, ` `, ` [ERROR ->]msg should contain only ph and pc tags`, ` `, - ` `, - `"): /some/file.xlf@6:16`, + `...`, + ``, ].join('\n')); }); @@ -677,13 +678,14 @@ describe( ].join('\n'); expectToFail('/some/file.xlf', XLIFF, /Missing required "equiv" attribute/, [ - `Missing required "equiv" attribute: ("`, - ` `, + `Error: Missing required "equiv" attribute:`, + `At /some/file.xlf@6:16:`, + `...`, ` `, ` [ERROR ->]`, ` `, - ` `, - `"): /some/file.xlf@6:16`, + `...`, + ``, ].join('\n')); }); }); diff --git a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts index 20ce00469565c..bc91e87d8e80b 100644 --- a/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts +++ b/packages/localize/src/tools/test/translate/translation_files/translation_parsers/xtb_translation_parser_spec.ts @@ -312,10 +312,14 @@ describe('XtbTranslationParser', () => { expect(result.translations['invalid']).toBeUndefined(); expect(result.diagnostics.messages).toContain({ type: 'warning', - message: - `Could not parse message with id "invalid" - perhaps it has an unrecognised ICU format?\n` + - `Error: Unexpected character "EOF" (Do you have an unescaped "{" in your template? Use "{{ '{' }}") to escape it.)\n` + - `Error: Invalid ICU message. Missing '}'.` + message: [ + `Could not parse message with id "invalid" - perhaps it has an unrecognised ICU format?`, + `Unexpected character "EOF" (Do you have an unescaped "{" in your template? Use "{{ '{' }}") to escape it.) ("id">{REGION_COUNT_1, plural, =0 {unused plural form} =1 {1 region} other {{REGION_COUNT_2} regions}}[ERROR ->]`, + `"): /some/file.xtb@2:124`, + `Invalid ICU message. Missing '}'. ("n>`, + ` {REGION_COUNT_1, plural, =0 {unused plural form} =1 {1 region} other [ERROR ->]{{REGION_COUNT_2} regions}}`, + `"): /some/file.xtb@2:97`, + ].join('\n') }); }); @@ -371,11 +375,14 @@ describe('XtbTranslationParser', () => { ].join('\n'); expectToFail('/some/file.xtb', XTB, /Invalid element found in message/, [ - `Invalid element found in message. ("`, + `Error: Invalid element found in message.`, + `At /some/file.xtb@2:4:`, + `...`, ` `, ` [ERROR ->]`, ` `, - `"): /some/file.xtb@2:4`, + `...`, + ``, ].join('\n')); }); @@ -387,9 +394,12 @@ describe('XtbTranslationParser', () => { ].join('\n'); expectToFail('/some/file.xtb', XTB, /required "name" attribute/gi, [ - `Missing required "name" attribute: ("`, + `Error: Missing required "name" attribute:`, + `At /some/file.xtb@1:29:`, + `...`, ` [ERROR ->]`, - `"): /some/file.xtb@1:29`, + `...`, + ``, ].join('\n')); }); });