From 57ead7aa53016fe4b5eaaffbb41fd434b3b74faf Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Sat, 24 Oct 2020 16:50:25 +0100 Subject: [PATCH] fix(localize): serialize all the message locations to XLIFF (#39411) Previously only the first message, for each id, was serialized which meant that additional message location information was lost. Now all the message locations are included in the serialized messages. Fixes #39330 PR Close #39411 --- .../src/extract/translation_files/utils.ts | 37 ++++++++ .../xliff1_translation_serializer.ts | 19 ++-- .../xliff2_translation_serializer.ts | 22 ++--- .../xliff1_translation_serializer_spec.ts | 95 +++++++++++++++++++ .../xliff2_translation_serializer_spec.ts | 85 +++++++++++++++++ 5 files changed, 237 insertions(+), 21 deletions(-) create mode 100644 packages/localize/src/tools/src/extract/translation_files/utils.ts diff --git a/packages/localize/src/tools/src/extract/translation_files/utils.ts b/packages/localize/src/tools/src/extract/translation_files/utils.ts new file mode 100644 index 0000000000..c2fa65c43f --- /dev/null +++ b/packages/localize/src/tools/src/extract/translation_files/utils.ts @@ -0,0 +1,37 @@ +/** + * @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 {ɵMessageId, ɵParsedMessage, ɵSourceLocation} from '@angular/localize'; + +/** + * Consolidate an array of messages into a map from message id to an array of messages with that id. + * + * @param messages the messages to consolidate. + * @param getMessageId a function that will compute the message id of a message. + */ +export function consolidateMessages( + messages: ɵParsedMessage[], + getMessageId: (message: ɵParsedMessage) => string): Map<ɵMessageId, ɵParsedMessage[]> { + const consolidateMessages = new Map<ɵMessageId, ɵParsedMessage[]>(); + for (const message of messages) { + const id = getMessageId(message); + if (!consolidateMessages.has(id)) { + consolidateMessages.set(id, [message]); + } else { + consolidateMessages.get(id)!.push(message); + } + } + return consolidateMessages; +} + +/** + * Does the given message have a location property? + */ +export function hasLocation(message: ɵParsedMessage): message is ɵParsedMessage& + {location: ɵSourceLocation} { + return message.location !== undefined; +} diff --git a/packages/localize/src/tools/src/extract/translation_files/xliff1_translation_serializer.ts b/packages/localize/src/tools/src/extract/translation_files/xliff1_translation_serializer.ts index 363000a557..78006a2980 100644 --- a/packages/localize/src/tools/src/extract/translation_files/xliff1_translation_serializer.ts +++ b/packages/localize/src/tools/src/extract/translation_files/xliff1_translation_serializer.ts @@ -11,6 +11,7 @@ import {ɵParsedMessage, ɵSourceLocation} from '@angular/localize'; import {FormatOptions, validateOptions} from './format_options'; import {extractIcuPlaceholders} from './icu_parsing'; import {TranslationSerializer} from './translation_serializer'; +import {consolidateMessages, hasLocation} from './utils'; import {XmlFile} from './xml_file'; /** This is the number of characters that a legacy Xliff 1.2 message id has. */ @@ -33,7 +34,7 @@ export class Xliff1TranslationSerializer implements TranslationSerializer { } serialize(messages: ɵParsedMessage[]): string { - const ids = new Set(); + const messageMap = consolidateMessages(messages, message => this.getMessageId(message)); const xml = new XmlFile(); xml.startTag('xliff', {'version': '1.2', 'xmlns': 'urn:oasis:names:tc:xliff:document:1.2'}); // NOTE: the `original` property is set to the legacy `ng2.template` value for backward @@ -50,21 +51,19 @@ export class Xliff1TranslationSerializer implements TranslationSerializer { ...this.formatOptions, }); xml.startTag('body'); - for (const message of messages) { - const id = this.getMessageId(message); - if (ids.has(id)) { - // Do not render the same message more than once - continue; - } - ids.add(id); + for (const [id, duplicateMessages] of messageMap.entries()) { + const message = duplicateMessages[0]; xml.startTag('trans-unit', {id, datatype: 'html'}); xml.startTag('source', {}, {preserveWhitespace: true}); this.serializeMessage(xml, message); xml.endTag('source', {preserveWhitespace: false}); - if (message.location) { - this.serializeLocation(xml, message.location); + + // Write all the locations + for (const {location} of duplicateMessages.filter(hasLocation)) { + this.serializeLocation(xml, location); } + if (message.description) { this.serializeNote(xml, 'description', message.description); } diff --git a/packages/localize/src/tools/src/extract/translation_files/xliff2_translation_serializer.ts b/packages/localize/src/tools/src/extract/translation_files/xliff2_translation_serializer.ts index 177ea75e69..fb926abfdb 100644 --- a/packages/localize/src/tools/src/extract/translation_files/xliff2_translation_serializer.ts +++ b/packages/localize/src/tools/src/extract/translation_files/xliff2_translation_serializer.ts @@ -11,6 +11,7 @@ import {ɵParsedMessage, ɵSourceLocation} from '@angular/localize'; import {FormatOptions, validateOptions} from './format_options'; import {extractIcuPlaceholders} from './icu_parsing'; import {TranslationSerializer} from './translation_serializer'; +import {consolidateMessages, hasLocation} from './utils'; import {XmlFile} from './xml_file'; /** This is the maximum number of characters that can appear in a legacy XLIFF 2.0 message id. */ @@ -33,7 +34,7 @@ export class Xliff2TranslationSerializer implements TranslationSerializer { } serialize(messages: ɵParsedMessage[]): string { - const ids = new Set(); + const messageMap = consolidateMessages(messages, message => this.getMessageId(message)); const xml = new XmlFile(); xml.startTag('xliff', { 'version': '2.0', @@ -48,24 +49,23 @@ export class Xliff2TranslationSerializer implements TranslationSerializer { // messages that come from a particular original file, and the translation file parsers may // not xml.startTag('file', {'id': 'ngi18n', 'original': 'ng.template', ...this.formatOptions}); - for (const message of messages) { - const id = this.getMessageId(message); - if (ids.has(id)) { - // Do not render the same message more than once - continue; - } - ids.add(id); + for (const [id, duplicateMessages] of messageMap.entries()) { + const message = duplicateMessages[0]; + xml.startTag('unit', {id}); - if (message.meaning || message.description || message.location) { + const messagesWithLocations = duplicateMessages.filter(hasLocation); + if (message.meaning || message.description || messagesWithLocations.length) { xml.startTag('notes'); - if (message.location) { - const {file, start, end} = message.location; + + // Write all the locations + for (const {location: {file, start, end}} of messagesWithLocations) { const endLineString = end !== undefined && end.line !== start.line ? `,${end.line + 1}` : ''; this.serializeNote( xml, 'location', `${this.fs.relative(this.basePath, file)}:${start.line + 1}${endLineString}`); } + if (message.description) { this.serializeNote(xml, 'description', message.description); } diff --git a/packages/localize/src/tools/test/extract/translation_files/xliff1_translation_serializer_spec.ts b/packages/localize/src/tools/test/extract/translation_files/xliff1_translation_serializer_spec.ts index 0ed91d10b3..9697a8cfda 100644 --- a/packages/localize/src/tools/test/extract/translation_files/xliff1_translation_serializer_spec.ts +++ b/packages/localize/src/tools/test/extract/translation_files/xliff1_translation_serializer_spec.ts @@ -135,6 +135,101 @@ runInEachFileSystem(() => { `\n`, ].join('\n')); }); + + it('should convert a set of parsed messages into an XML string', () => { + const messageLocation1: ɵSourceLocation = { + start: {line: 0, column: 5}, + end: {line: 0, column: 10}, + file: absoluteFrom('/project/file-1.ts'), + text: 'message text' + }; + + const messageLocation2: ɵSourceLocation = { + start: {line: 3, column: 2}, + end: {line: 4, column: 7}, + file: absoluteFrom('/project/file-2.ts'), + text: 'message text' + }; + + const messageLocation3: ɵSourceLocation = { + start: {line: 0, column: 5}, + end: {line: 0, column: 10}, + file: absoluteFrom('/project/file-3.ts'), + text: 'message text' + }; + + const messageLocation4: ɵSourceLocation = { + start: {line: 3, column: 2}, + end: {line: 4, column: 7}, + file: absoluteFrom('/project/file-4.ts'), + text: 'message text' + }; + + const messages: ɵParsedMessage[] = [ + mockMessage('1234', ['message text'], [], {location: messageLocation1}), + mockMessage('1234', ['message text'], [], {location: messageLocation2}), + mockMessage('1234', ['message text'], [], { + location: messageLocation3, + legacyIds: ['87654321FEDCBA0987654321FEDCBA0987654321', '563965274788097516'] + }), + mockMessage( + '1234', ['message text'], [], {location: messageLocation4, customId: 'other'}), + ]; + const serializer = new Xliff1TranslationSerializer( + 'xx', absoluteFrom('/project'), useLegacyIds, options); + const output = serializer.serialize(messages); + + // Note that in this test the third message will match the first two if legacyIds is + // false. Otherwise it will be a separate message on its own. + + expect(output).toEqual([ + ``, + ``, + ` `, + ` `, + ` `, + ` message text`, + ` `, + ` file-1.ts`, + ` 1`, + ` `, + ` `, + ` file-2.ts`, + ` 4,5`, + ` `, + ...useLegacyIds ? + [] : + [ + ` `, + ` file-3.ts`, + ` 1`, + ` `, + ], + ` `, + ...useLegacyIds ? + [ + ` `, + ` message text`, + ` `, + ` file-3.ts`, + ` 1`, + ` `, + ` `, + ] : + [], + ` `, + ` message text`, + ` `, + ` file-4.ts`, + ` 4,5`, + ` `, + ` `, + ` `, + ` `, + `\n`, + ].join('\n')); + }); }); }); }); diff --git a/packages/localize/src/tools/test/extract/translation_files/xliff2_translation_serializer_spec.ts b/packages/localize/src/tools/test/extract/translation_files/xliff2_translation_serializer_spec.ts index 5d424ef4a7..832d517ff7 100644 --- a/packages/localize/src/tools/test/extract/translation_files/xliff2_translation_serializer_spec.ts +++ b/packages/localize/src/tools/test/extract/translation_files/xliff2_translation_serializer_spec.ts @@ -158,6 +158,91 @@ runInEachFileSystem(() => { `\n`, ].join('\n')); }); + + it('should convert a set of parsed messages into an XML string', () => { + const messageLocation1: ɵSourceLocation = { + start: {line: 0, column: 5}, + end: {line: 0, column: 10}, + file: absoluteFrom('/project/file-1.ts'), + text: 'message text' + }; + + const messageLocation2: ɵSourceLocation = { + start: {line: 3, column: 2}, + end: {line: 4, column: 7}, + file: absoluteFrom('/project/file-2.ts'), + text: 'message text' + }; + + const messageLocation3: ɵSourceLocation = { + start: {line: 0, column: 5}, + end: {line: 0, column: 10}, + file: absoluteFrom('/project/file-3.ts'), + text: 'message text' + }; + + const messageLocation4: ɵSourceLocation = { + start: {line: 3, column: 2}, + end: {line: 4, column: 7}, + file: absoluteFrom('/project/file-4.ts'), + text: 'message text' + }; + + const messages: ɵParsedMessage[] = [ + mockMessage('1234', ['message text'], [], {location: messageLocation1}), + mockMessage('1234', ['message text'], [], {location: messageLocation2}), + mockMessage('1234', ['message text'], [], { + location: messageLocation3, + legacyIds: ['87654321FEDCBA0987654321FEDCBA0987654321', '563965274788097516'] + }), + mockMessage( + '1234', ['message text'], [], {location: messageLocation4, customId: 'other'}), + ]; + const serializer = new Xliff2TranslationSerializer( + 'xx', absoluteFrom('/project'), useLegacyIds, options); + const output = serializer.serialize(messages); + + // Note that in this test the third message will match the first two if legacyIds is + // false. Otherwise it will be a separate message on its own. + + expect(output).toEqual([ + ``, + ``, + ` `, + ` `, + ` `, + ` file-1.ts:1`, + ` file-2.ts:4,5`, + ...useLegacyIds ? [] : [' file-3.ts:1'], + ` `, + ` `, + ` message text`, + ` `, + ` `, + ...useLegacyIds ? + [ + ` `, + ` `, + ` file-3.ts:1`, + ` `, + ` `, + ` message text`, + ` `, + ` `, + ] : + [], + ` `, + ` `, + ` file-4.ts:4,5`, + ` `, + ` `, + ` message text`, + ` `, + ` `, + ` `, + `\n`, + ].join('\n')); + }); }); }); });