From cf9a47ba53738bf3b8ce282938211370e946bbf9 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 15 Jul 2020 20:08:18 +0100 Subject: [PATCH] feat(localize): allow duplicate messages to be handled during extraction (#38082) Previously, the i18n message extractor just quietly ignored messages that it extracted that had the same id. It can be helpful to identify these to track down messages that have the same id but different message text. Now the messages are checked for duplicate ids with different message text. Any that are found can be reported based on the new `--duplicateMessageHandling` command line option (or `duplicateMessageHandling` API options property). * "ignore" - no action is taken * "warning" - a diagnostic warning is written to the logger * "error" - the extractor throws an error and exits Fixes #38077 PR Close #38082 --- .../src/tools/src/extract/duplicates.ts | 58 ++++++++++++ .../localize/src/tools/src/extract/main.ts | 34 ++++++- .../src/tools/src/source_file_utils.ts | 7 ++ .../test/extract/integration/main_spec.ts | 88 ++++++++++++++++++- .../integration/test_files/duplicate.js | 7 ++ 5 files changed, 187 insertions(+), 7 deletions(-) create mode 100644 packages/localize/src/tools/src/extract/duplicates.ts create mode 100644 packages/localize/src/tools/test/extract/integration/test_files/duplicate.js diff --git a/packages/localize/src/tools/src/extract/duplicates.ts b/packages/localize/src/tools/src/extract/duplicates.ts new file mode 100644 index 0000000000..dfe1eb6f5e --- /dev/null +++ b/packages/localize/src/tools/src/extract/duplicates.ts @@ -0,0 +1,58 @@ +/** + * @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 {AbsoluteFsPath, FileSystem} from '@angular/compiler-cli/src/ngtsc/file_system'; +import {ɵMessageId, ɵParsedMessage} from '@angular/localize'; + +import {DiagnosticHandlingStrategy, Diagnostics} from '../diagnostics'; +import {serializeLocationPosition} from '../source_file_utils'; + +/** + * Check each of the given `messages` to find those that have the same id but different message + * text. Add diagnostics messages for each of these duplicate messages to the given `diagnostics` + * object (as necessary). + */ +export function checkDuplicateMessages( + fs: FileSystem, messages: ɵParsedMessage[], + duplicateMessageHandling: DiagnosticHandlingStrategy, basePath: AbsoluteFsPath): Diagnostics { + const diagnostics = new Diagnostics(); + if (duplicateMessageHandling === 'ignore') return diagnostics; + + const messageMap = new Map<ɵMessageId, ɵParsedMessage[]>(); + for (const message of messages) { + if (messageMap.has(message.id)) { + messageMap.get(message.id)!.push(message); + } else { + messageMap.set(message.id, [message]); + } + } + + for (const duplicates of messageMap.values()) { + if (duplicates.length <= 1) continue; + if (duplicates.every(message => message.text === duplicates[0].text)) continue; + + const diagnosticMessage = `Duplicate messages with id "${duplicates[0].id}":\n` + + duplicates.map(message => serializeMessage(fs, basePath, message)).join('\n'); + diagnostics.add(duplicateMessageHandling, diagnosticMessage); + } + + return diagnostics; +} + +/** + * Serialize the given `message` object into a string. + */ +function serializeMessage( + fs: FileSystem, basePath: AbsoluteFsPath, message: ɵParsedMessage): string { + if (message.location === undefined) { + return ` - "${message.text}"`; + } else { + const locationFile = fs.relative(basePath, message.location.file); + const locationPosition = serializeLocationPosition(message.location); + return ` - "${message.text}" : ${locationFile}:${locationPosition}`; + } +} diff --git a/packages/localize/src/tools/src/extract/main.ts b/packages/localize/src/tools/src/extract/main.ts index 517146945b..e0a41f0528 100644 --- a/packages/localize/src/tools/src/extract/main.ts +++ b/packages/localize/src/tools/src/extract/main.ts @@ -11,6 +11,10 @@ import {ConsoleLogger, Logger, LogLevel} from '@angular/compiler-cli/src/ngtsc/l import {ɵParsedMessage} from '@angular/localize'; import * as glob from 'glob'; import * as yargs from 'yargs'; + +import {DiagnosticHandlingStrategy, Diagnostics} from '../diagnostics'; + +import {checkDuplicateMessages} from './duplicates'; import {MessageExtractor} from './extraction'; import {TranslationSerializer} from './translation_files/translation_serializer'; import {SimpleJsonTranslationSerializer} from './translation_files/json_translation_serializer'; @@ -68,6 +72,12 @@ if (require.main === module) { describe: 'Whether to use the legacy id format for messages that were extracted from Angular templates.' }) + .option('d', { + alias: 'duplicateMessageHandling', + describe: 'How to handle messages with the same id but different text.', + choices: ['error', 'warning', 'ignore'], + default: 'warning', + }) .strict() .help() .parse(args); @@ -79,6 +89,7 @@ if (require.main === module) { const sourceFilePaths = glob.sync(options['source'], {cwd: rootPath, nodir: true}); const logLevel = options['loglevel'] as (keyof typeof LogLevel) | undefined; const logger = new ConsoleLogger(logLevel ? LogLevel[logLevel] : LogLevel.warn); + const duplicateMessageHandling: DiagnosticHandlingStrategy = options['d']; extractTranslations({ @@ -90,6 +101,7 @@ if (require.main === module) { logger, useSourceMaps: options['useSourceMaps'], useLegacyIds: options['useLegacyIds'], + duplicateMessageHandling, }); } @@ -129,6 +141,10 @@ export interface ExtractTranslationsOptions { * Whether to use the legacy id format for messages that were extracted from Angular templates */ useLegacyIds: boolean; + /** + * How to handle messages with the same id but not the same text. + */ + duplicateMessageHandling: DiagnosticHandlingStrategy; } export function extractTranslations({ @@ -139,22 +155,32 @@ export function extractTranslations({ outputPath: output, logger, useSourceMaps, - useLegacyIds + useLegacyIds, + duplicateMessageHandling, }: ExtractTranslationsOptions) { const fs = getFileSystem(); - const extractor = - new MessageExtractor(fs, logger, {basePath: fs.resolve(rootPath), useSourceMaps}); + const basePath = fs.resolve(rootPath); + const extractor = new MessageExtractor(fs, logger, {basePath, useSourceMaps}); const messages: ɵParsedMessage[] = []; for (const file of sourceFilePaths) { messages.push(...extractor.extractMessages(file)); } + const diagnostics = checkDuplicateMessages(fs, messages, duplicateMessageHandling, basePath); + if (diagnostics.hasErrors) { + throw new Error(diagnostics.formatDiagnostics('Failed to extract messages')); + } + const outputPath = fs.resolve(rootPath, output); const serializer = getSerializer(format, sourceLocale, fs.dirname(outputPath), useLegacyIds); const translationFile = serializer.serialize(messages); fs.ensureDir(fs.dirname(outputPath)); fs.writeFile(outputPath, translationFile); + + if (diagnostics.messages.length) { + logger.warn(diagnostics.formatDiagnostics('Messages extracted with warnings')); + } } export function getSerializer( @@ -175,4 +201,4 @@ export function getSerializer( return new SimpleJsonTranslationSerializer(sourceLocale); } throw new Error(`No translation serializer can handle the provided format: ${format}`); -} \ No newline at end of file +} diff --git a/packages/localize/src/tools/src/source_file_utils.ts b/packages/localize/src/tools/src/source_file_utils.ts index 0a5f58ba0b..ed20ac156a 100644 --- a/packages/localize/src/tools/src/source_file_utils.ts +++ b/packages/localize/src/tools/src/source_file_utils.ts @@ -365,6 +365,13 @@ export function getLocation(startPath: NodePath, endPath?: NodePath): ɵSourceLo }; } +export function serializeLocationPosition(location: ɵSourceLocation): string { + const endLineString = location.end !== undefined && location.end.line !== location.start.line ? + `,${location.end.line + 1}` : + ''; + return `${location.start.line + 1}${endLineString}`; +} + function getFileFromPath(path: NodePath|undefined): AbsoluteFsPath|null { const opts = path?.hub.file.opts; return opts?.filename ? diff --git a/packages/localize/src/tools/test/extract/integration/main_spec.ts b/packages/localize/src/tools/test/extract/integration/main_spec.ts index bb7d19c2ee..7e5fd37fc6 100644 --- a/packages/localize/src/tools/test/extract/integration/main_spec.ts +++ b/packages/localize/src/tools/test/extract/integration/main_spec.ts @@ -7,7 +7,6 @@ */ import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system'; import {runInEachFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing'; -import {Logger} from '@angular/compiler-cli/src/ngtsc/logging'; import {MockLogger} from '@angular/compiler-cli/src/ngtsc/logging/testing'; import {loadTestDirectory} from '@angular/compiler-cli/test/helpers'; @@ -15,7 +14,7 @@ import {extractTranslations} from '../../../src/extract/main'; runInEachFileSystem(() => { let fs: FileSystem; - let logger: Logger; + let logger: MockLogger; let rootPath: AbsoluteFsPath; let outputPath: AbsoluteFsPath; let sourceFilePath: AbsoluteFsPath; @@ -40,12 +39,13 @@ runInEachFileSystem(() => { extractTranslations({ rootPath, sourceLocale: 'en', - sourceFilePaths: [], + sourceFilePaths: [textFile1, textFile2], format: 'json', outputPath, logger, useSourceMaps: false, useLegacyIds: false, + duplicateMessageHandling: 'ignore', }); expect(fs.readFile(outputPath)).toEqual([ `{`, @@ -65,6 +65,7 @@ runInEachFileSystem(() => { logger, useSourceMaps: false, useLegacyIds: false, + duplicateMessageHandling: 'ignore', }); expect(fs.readFile(outputPath)).toEqual([ `{`, @@ -87,6 +88,7 @@ runInEachFileSystem(() => { logger, useSourceMaps: false, useLegacyIds: false, + duplicateMessageHandling: 'ignore', }); expect(fs.readFile(outputPath)).toEqual([ ``, @@ -128,6 +130,7 @@ runInEachFileSystem(() => { logger, useSourceMaps: false, useLegacyIds: false, + duplicateMessageHandling: 'ignore', }); expect(fs.readFile(outputPath)).toEqual([ ``, @@ -164,6 +167,7 @@ runInEachFileSystem(() => { logger, useSourceMaps: false, useLegacyIds: false, + duplicateMessageHandling: 'ignore', }); expect(fs.readFile(outputPath)).toEqual([ ``, @@ -197,6 +201,7 @@ runInEachFileSystem(() => { logger, useSourceMaps: true, useLegacyIds: false, + duplicateMessageHandling: 'ignore', }); expect(fs.readFile(outputPath)).toEqual([ ``, @@ -224,5 +229,82 @@ runInEachFileSystem(() => { ].join('\n')); }); } + + describe('[duplicateMessageHandling]', () => { + it('should throw if set to "error"', () => { + expect(() => extractTranslations({ + rootPath, + sourceLocale: 'en-GB', + sourceFilePaths: [fs.resolve(rootPath, 'test_files/duplicate.js')], + format: 'json', + outputPath, + logger, + useSourceMaps: false, + useLegacyIds: false, + duplicateMessageHandling: 'error', + })) + .toThrowError( + `Failed to extract messages\n` + + `ERRORS:\n` + + ` - Duplicate messages with id "message-2":\n` + + ` - "message contents" : test_files/duplicate.js:6\n` + + ` - "different message contents" : test_files/duplicate.js:7`); + expect(fs.exists(outputPath)).toBe(false); + }); + + it('should log to the logger if set to "warning"', () => { + extractTranslations({ + rootPath, + sourceLocale: 'en-GB', + sourceFilePaths: [fs.resolve(rootPath, 'test_files/duplicate.js')], + format: 'json', + outputPath, + logger, + useSourceMaps: false, + useLegacyIds: false, + duplicateMessageHandling: 'warning', + }); + expect(logger.logs.warn).toEqual([ + ['Messages extracted with warnings\n' + + `WARNINGS:\n` + + ` - Duplicate messages with id "message-2":\n` + + ` - "message contents" : test_files/duplicate.js:6\n` + + ` - "different message contents" : test_files/duplicate.js:7`] + ]); + expect(fs.readFile(outputPath)).toEqual([ + `{`, + ` "locale": "en-GB",`, + ` "translations": {`, + ` "message-1": "message {$PH} contents",`, + ` "message-2": "different message contents"`, + ` }`, + `}`, + ].join('\n')); + }); + + it('should not log to the logger if set to "ignore"', () => { + extractTranslations({ + rootPath, + sourceLocale: 'en-GB', + sourceFilePaths: [fs.resolve(rootPath, 'test_files/duplicate.js')], + format: 'json', + outputPath, + logger, + useSourceMaps: false, + useLegacyIds: false, + duplicateMessageHandling: 'ignore', + }); + expect(logger.logs.warn).toEqual([]); + expect(fs.readFile(outputPath)).toEqual([ + `{`, + ` "locale": "en-GB",`, + ` "translations": {`, + ` "message-1": "message {$PH} contents",`, + ` "message-2": "different message contents"`, + ` }`, + `}`, + ].join('\n')); + }); + }); }); }); diff --git a/packages/localize/src/tools/test/extract/integration/test_files/duplicate.js b/packages/localize/src/tools/test/extract/integration/test_files/duplicate.js new file mode 100644 index 0000000000..b25f64566d --- /dev/null +++ b/packages/localize/src/tools/test/extract/integration/test_files/duplicate.js @@ -0,0 +1,7 @@ +// Different interpolation values will not affect message text +const a = $localize`:@@message-1:message ${10} contents`; +const b = $localize`:@@message-1:message ${20} contents`; + +// But different actual text content will +const c = $localize`:@@message-2:message contents`; +const d = $localize`:@@message-2:different message contents`; \ No newline at end of file