From 317ad64ae6373006cdf2832d513fc905f5cbddfb Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 20 Oct 2017 18:38:22 +0100 Subject: [PATCH 1/3] Make the gen-i18n script validate _t calls And throw a massive tantrum if you've messed up your format strings. Because broken format strings making their way into the app cause it to throw exceptions. --- scripts/gen-i18n.js | 62 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/scripts/gen-i18n.js b/scripts/gen-i18n.js index 609c6b00c5..56f64d2154 100755 --- a/scripts/gen-i18n.js +++ b/scripts/gen-i18n.js @@ -35,6 +35,7 @@ const estreeWalker = require('estree-walker'); const TRANSLATIONS_FUNCS = ['_t', '_td', '_tJsx']; const INPUT_TRANSLATIONS_FILE = 'src/i18n/strings/en_EN.json'; +const OUTPUT_FILE = 'src/i18n/strings/en_EN.json'; // NB. The sync version of walk is broken for single files so we walk // all of res rather than just res/home.html. @@ -73,6 +74,41 @@ function getTKey(arg) { return null; } +function getFormatStrings(str) { + // Match anything that starts with % + // We could make a regex that matched the full placeholder, but this + // would just not match invalid placeholders and so wouldn't help us + // detect the invalid ones. + // Also note that for simplicity, this just matches a % character and then + // anything up to the next % character (or a single %, or end of string). + const formatStringRe = /%([^%]+|%|$)/g; + const formatStrings = new Set(); + + let match; + while ( (match = formatStringRe.exec(str)) !== null) { + const placeholder = match[1]; // Minus the leading '%' + if (placeholder === '%') continue; // Literal % is %% + + const placeholderMatch = placeholder.match(/^\((.*?)\)(.)/); + if (placeholderMatch === null) { + throw new Error("Invalid format specifier: '"+match[0]+"'"); + } + if (placeholderMatch.length < 3) { + throw new Error("Malformed format specifier"); + } + const placeHolderName = placeholderMatch[1]; + const placeHolderFormat = placeholderMatch[2]; + + if (placeHolderFormat !== 's') { + throw new Error(`'${placeHolderFormat}' used as format character: you probably didn't mean this`); + } + + formatStrings.add(placeHolderName); + } + + return formatStrings; +} + function getTranslationsJs(file) { const tree = flowParser.parse(fs.readFileSync(file, { encoding: 'utf8' }), FLOW_PARSER_OPTS); @@ -89,6 +125,28 @@ function getTranslationsJs(file) { // had to use a _td to compensate) so is expected. if (tKey === null) return; + // check the format string against the args + // We only check _t: _tJsx is much more complex and _td has no args + if (node.callee.name === '_t') { + try { + const placeholders = getFormatStrings(tKey); + for (const placeholder of placeholders) { + if (node.arguments.length < 2) { + throw new Error(`Placeholder found ('${placeholder}') but no substitutions given`); + } + const value = getObjectValue(node.arguments[1], placeholder); + if (value === null) { + throw new Error(`No value found for placeholder '${placeholder}'`); + } + } + } catch (e) { + console.log(); + console.error(`ERROR: ${file}:${node.loc.start.line} ${tKey}`); + console.error(e); + process.exit(1); + } + } + let isPlural = false; if (node.arguments.length > 1 && node.arguments[1].type == 'ObjectExpression') { const countVal = getObjectValue(node.arguments[1], 'count'); @@ -182,7 +240,9 @@ for (const tr of translatables) { } fs.writeFileSync( - "src/i18n/strings/en_EN.json", + OUTPUT_FILE, JSON.stringify(trObj, translatables.values(), 4) + "\n" ); +console.log(); +console.log(`Wrote ${translatables.size} strings to ${OUTPUT_FILE}`); From 8d1aea5b2ec5fb542282baa1daa76b6795f1ce0d Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 23 Oct 2017 10:18:29 +0100 Subject: [PATCH 2/3] Misc PR review fixes --- scripts/gen-i18n.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/scripts/gen-i18n.js b/scripts/gen-i18n.js index 56f64d2154..ff5b14f593 100755 --- a/scripts/gen-i18n.js +++ b/scripts/gen-i18n.js @@ -85,7 +85,7 @@ function getFormatStrings(str) { const formatStrings = new Set(); let match; - while ( (match = formatStringRe.exec(str)) !== null) { + while ( (match = formatStringRe.exec(str) ) !== null) { const placeholder = match[1]; // Minus the leading '%' if (placeholder === '%') continue; // Literal % is %% @@ -96,14 +96,14 @@ function getFormatStrings(str) { if (placeholderMatch.length < 3) { throw new Error("Malformed format specifier"); } - const placeHolderName = placeholderMatch[1]; - const placeHolderFormat = placeholderMatch[2]; + const placeholderName = placeholderMatch[1]; + const placeholderFormat = placeholderMatch[2]; - if (placeHolderFormat !== 's') { - throw new Error(`'${placeHolderFormat}' used as format character: you probably didn't mean this`); + if (placeholderFormat !== 's') { + throw new Error(`'${placeholderFormat}' used as format character: you probably meant 's'`); } - formatStrings.add(placeHolderName); + formatStrings.add(placeholderName); } return formatStrings; @@ -142,7 +142,6 @@ function getTranslationsJs(file) { } catch (e) { console.log(); console.error(`ERROR: ${file}:${node.loc.start.line} ${tKey}`); - console.error(e); process.exit(1); } } From 54458f143881247082c14d074414012546a6d19e Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 23 Oct 2017 14:02:58 +0100 Subject: [PATCH 3/3] Actually even out brackets --- scripts/gen-i18n.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/gen-i18n.js b/scripts/gen-i18n.js index ff5b14f593..d58bce2e63 100755 --- a/scripts/gen-i18n.js +++ b/scripts/gen-i18n.js @@ -85,7 +85,7 @@ function getFormatStrings(str) { const formatStrings = new Set(); let match; - while ( (match = formatStringRe.exec(str) ) !== null) { + while ( (match = formatStringRe.exec(str)) !== null ) { const placeholder = match[1]; // Minus the leading '%' if (placeholder === '%') continue; // Literal % is %%