DevTools: Report missing description and placeholder example as localizability violation in presubmit
This patch adds a localizability check to the presubmit script that reports
errors for empty descriptions, after https://crrev.com/c/1688716 adds auto-
generated descriptions.
Example error messages:
third_party/blink/renderer/devtools/front_end/accessibility/accessibility_strings.grdp
Line 36: missing description for message with the name "IDS_DEVTOOLS_185551542d4a950d6ed4a90e0875dfde"
third_party/blink/renderer/devtools/front_end/animation/animation_strings.grdp Line 10:
missing <ex> in <ph> tag with the name "BUTTON_TEXTCONTENT"
This patch also improves error reporting by changing full file path to partial
path from src/ so that it's more consistent with the presubmit error style.
Note: there may be new strings added without a description and/or placeholder
example while this patch is on review. In that case I will fix them so the
check can be enabled without any problem.
Bug: 941561
Change-Id: If609d234b67acb76dfd9c66e2f8880d9875034cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1693028
Commit-Queue: Mandy Chen <mandy.chen@microsoft.com>
Reviewed-by: Joel Einbinder <einbinder@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#681975}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 1404bfcfd225f2963874e37299d890b1f3119e72
diff --git a/scripts/check_localizability.js b/scripts/check_localizability.js
index e55d741..000a294 100644
--- a/scripts/check_localizability.js
+++ b/scripts/check_localizability.js
@@ -4,6 +4,7 @@
'use strict';
// Description: Scans for localizability violations in the DevTools front-end.
+// Checks all .grdp files and reports messages without descriptions and placeholder examples.
// Audits all Common.UIString(), UI.formatLocalized(), and ls`` calls and
// checks for misuses of concatenation and conditionals. It also looks for
// specific arguments to functions that are expected to be a localized string.
@@ -40,18 +41,16 @@
try {
let filePaths = [];
- if (process.argv[2] === '-a') {
- const frontendPath = path.resolve(__dirname, '..', 'front_end');
- await localizationUtils.getFilesFromDirectory(frontendPath, filePaths, ['.js']);
- } else {
+ const frontendPath = path.resolve(__dirname, '..', 'front_end');
+ let filePathPromises = [localizationUtils.getFilesFromDirectory(frontendPath, filePaths, ['.grdp'])];
+ if (process.argv[2] === '-a')
+ filePathPromises.push(localizationUtils.getFilesFromDirectory(frontendPath, filePaths, ['.js']));
+ else
filePaths = process.argv.slice(2);
- }
+ await Promise.all(filePathPromises);
- const promises = [];
- for (const filePath of filePaths)
- promises.push(auditFileForLocalizability(filePath, errors));
-
- await Promise.all(promises);
+ const auditFilePromises = filePaths.map(filePath => auditFileForLocalizability(filePath, errors));
+ await Promise.all(auditFilePromises);
} catch (err) {
console.log(err);
process.exit(1);
@@ -130,7 +129,7 @@
if (hasConcatenationViolation) {
const code = escodegen.generate(node);
addError(
- `${filePath}${
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)}${
localizationUtils.getLocationMessage(
node.loc)}: string concatenation should be changed to variable substitution with ls: ${code}`,
errors);
@@ -167,8 +166,9 @@
order = `${argumentIndex + 1}th`;
}
addError(
- `${filePath}${localizationUtils.getLocationMessage(node.loc)}: ${order} argument to ${
- functionName}() should be localized: ${escodegen.generate(node)}`,
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)}${
+ localizationUtils.getLocationMessage(
+ node.loc)}: ${order} argument to ${functionName}() should be localized: ${escodegen.generate(node)}`,
errors);
}
}
@@ -207,13 +207,14 @@
if (firstArgType !== esprimaTypes.LITERAL && firstArgType !== esprimaTypes.TEMP_LITERAL &&
firstArgType !== esprimaTypes.IDENTIFIER && !excludeErrors.includes(code)) {
addError(
- `${filePath}${localizationUtils.getLocationMessage(node.loc)}: first argument to call should be a string: ${
- code}`,
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)}${
+ localizationUtils.getLocationMessage(node.loc)}: first argument to call should be a string: ${code}`,
errors);
}
if (includesConditionalExpression(node.arguments.slice(1))) {
addError(
- `${filePath}${localizationUtils.getLocationMessage(node.loc)}: conditional(s) found in ${
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)}${
+ localizationUtils.getLocationMessage(node.loc)}: conditional(s) found in ${
code}. Please extract conditional(s) out of the localization call.`,
errors);
}
@@ -221,7 +222,8 @@
case 'Tagged Template':
if (includesConditionalExpression(node.quasi.expressions)) {
addError(
- `${filePath}${localizationUtils.getLocationMessage(node.loc)}: conditional(s) found in ${
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)}${
+ localizationUtils.getLocationMessage(node.loc)}: conditional(s) found in ${
code}. Please extract conditional(s) out of the localization call.`,
errors);
}
@@ -238,8 +240,50 @@
}
}
+function auditGrdpFile(filePath, fileContent, errors) {
+ function reportMissingPlaceholderExample(messageContent, lineNumber) {
+ const phRegex = /<ph[^>]*name="([^"]*)">\$\d(s|d|\.\df)(?!<ex>)<\/ph>/gms;
+ let match;
+ // ph tag that contains $1.2f format placeholder without <ex>
+ // match[0]: full match
+ // match[1]: ph name
+ while ((match = phRegex.exec(messageContent)) !== null) {
+ addError(
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)} Line ${
+ lineNumber +
+ localizationUtils.lineNumberOfIndex(
+ messageContent, match.index)}: missing <ex> in <ph> tag with the name "${match[1]}"`,
+ errors);
+ }
+ }
+
+ function reportMissingDescriptionAndPlaceholderExample() {
+ const messageRegex = /<message[^>]*name="([^"]*)"[^>]*desc="([^"]*)"[^>]*>\s*\n(.*?)<\/message>/gms;
+ let match;
+ // match[0]: full match
+ // match[1]: message IDS_ key
+ // match[2]: description
+ // match[3]: message content
+ while ((match = messageRegex.exec(fileContent)) !== null) {
+ const lineNumber = localizationUtils.lineNumberOfIndex(fileContent, match.index);
+ if (match[2].trim() === '') {
+ addError(
+ `${localizationUtils.getRelativeFilePathFromSrc(filePath)} Line ${
+ lineNumber}: missing description for message with the name "${match[1]}"`,
+ errors);
+ }
+ reportMissingPlaceholderExample(match[3], lineNumber);
+ }
+ }
+
+ reportMissingDescriptionAndPlaceholderExample();
+}
+
async function auditFileForLocalizability(filePath, errors) {
const fileContent = await localizationUtils.parseFileContent(filePath);
+ if (path.extname(filePath) === '.grdp')
+ return auditGrdpFile(filePath, fileContent, errors);
+
const ast = esprima.parse(fileContent, {loc: true});
const relativeFilePath = localizationUtils.getRelativeFilePathFromSrc(filePath);