Issue 6171 - create CSV exporter and importer for translations

csv-export.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,

[saroyanm, 2018/02/28 20:57:34]

Moving this file to the buildtools directory still needs to be addressed ->
https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1

[Thomas Greiner, 2018/03/19 18:28:04]

What does this have to do with buildtools? I was talking about creating a
"build" directory and moving this script there so that it's not lying around in
the repo's root directory with all the other files.

[Thomas Greiner, 2018/03/19 18:54:13]

Mind mentioning this script in the README? Preferrably, including a small
example of the CSV file format it imports/exports.

[saroyanm, 2018/04/26 17:53:52]

Acknowledged, I'll move this to the "build" directory.

[saroyanm, 2018/04/26 17:53:52]

Good point. I'll add information in the README as well.

[saroyanm, 2018/05/04 13:51:10]

Done.

[saroyanm, 2018/05/04 13:51:10]

Done.

+ * Copyright (C) 2006-present eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+const fs = require("fs");
+const {exec} = require("child_process");
+const path = require("path");
+const csv = require("csv");
+const {promisify} = require("util");
+const csvParser = promisify(csv.parse);
+
+
+const localesDir = "locale";
+const defaultLocale = "en_US";
+
+// ex.: desktop-options.json
+let fileNames = [];
+// List of all available locale codes
+let locales = [];
+
+let headers = ["StringID", "Description", "Placeholders", defaultLocale];
+let outputFileName = "translations-{repo}-{hash}.csv";
+
+/**
+ * Export existing translation - files into CSV file
+ * @param  {string[]} [filesFilter] - fileNames filter, if omitted all files 
+ *                                    will be exported
+ */
+function exportTranslations(filesFilter)
+{
+  let mercurialCommands = [];
+  // Get Hash
+  mercurialCommands.push(executeMercurial(["id", "-i"]));
+  // Get repo path
+  mercurialCommands.push(executeMercurial(["paths", "default"]));
+  Promise.all(mercurialCommands).then((outputs) =>
+  {
+    // Remove line endings and "+" sign from the end of the hash
+    let [hash, filePath] = outputs.map((item) => item.replace(/\+\n|\n$/, ""));
+    // Update name of the file to be output
+    outputFileName = outputFileName.replace("{hash}", hash);
+    outputFileName = outputFileName.replace("{repo}", path.basename(filePath));
+
+    // Read all available locales and default files
+    return Promise.all([readDir(path.join(localesDir, defaultLocale)),

[saroyanm, 2018/05/04 13:51:08]

I think we should use module like glob -> https://www.npmjs.com/package/glob 

I'm currently reading locales and original en_US folder and then trying to
construct the path, that why I have to consider the case where the file doesn't
exist, with glob I can easily extract all the json file paths and read them
without complex recursive directory reading implementation and my initial bogus
solution.


(Done)

[Thomas Greiner, 2018/05/07 15:16:33]

Acknowledged.

+                        readDir(localesDir)]);
+  }).then((files) =>
+  {
+    [fileNames, locales] = files;
+    // Filter files
+    if (filesFilter.length)
+      fileNames = fileNames.filter((item) => filesFilter.includes(item));
+
+    let readJsonPromises = [];
+    for(let fileName of fileNames)
+    {
+      for(let locale of locales)
+      {
+        readJsonPromises.push(readJson(locale, fileName));
+      }
+    }
+
+    // Reading all existing translations files
+    return Promise.all(readJsonPromises);
+  }).then(csvFromJsonFileObjects);
+}
+
+/**
+ * Creating Matrix which reflects output CSV file
+ * @param  {Array} fileObjects - array of file objects created by readJson
+ * @return {Array} Matrix
+ */
+function csvFromJsonFileObjects(fileObjects)
+{
+  // Create Object tree from the Objects array, for easier search
+  // ex.: {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
+  let dataTreeObj = fileObjects.reduce((accumulator, fileObject) =>
+  {
+    if (!fileObject)
+      return accumulator;
+
+    let {fileName, locale} = fileObject;
+    if (!accumulator[fileName])
+    {
+      accumulator[fileName] = {};
+    }
+    accumulator[fileName][locale] = fileObject.strings;
+    return accumulator;
+  }, {});
+
+  // Create two dimensional strings array that reflects CSV structure
+  let translationLocales = locales.filter((locale) => locale != defaultLocale);
+  let csvArray = [headers.concat(translationLocales)];
+  for (let fileName of fileNames)

[saroyanm, 2018/05/04 13:51:10]

I have this information in fileObjects, I shouldn't use separate array to read
filenames and locales.

(Done)

[Thomas Greiner, 2018/05/07 15:16:33]

Acknowledged.

+  {
+    csvArray.push([fileName]);
+    let strings = dataTreeObj[fileName][defaultLocale];
+    for (let stringID of Object.keys(strings))
+    {
+      let fileObj = dataTreeObj[fileName];
+      let {description, message, placeholders} = strings[stringID];
+      let row = [stringID, description || "", JSON.stringify(placeholders),
+                  message];
+
+      for (let locale of translationLocales)
+      {
+        let localeFileObj = fileObj[locale];
+        let isTranslated = !!(localeFileObj && localeFileObj[stringID]);
+        row.push(isTranslated ? localeFileObj[stringID].message : "");
+      }
+      csvArray.push(row);
+    }
+  }
+  arrayToCsv(csvArray);
+}
+
+/**
+ * Import strings from the CSV file
+ * @param  {string} filePath - CSV file path to import from
+ */
+function importTranslations(filePath)
+{
+  readFile(filePath).then((fileObjects) =>
+  {
+    return csvParser(fileObjects, {relax_column_count: true});

[Thomas Greiner, 2018/03/19 18:54:13]

Why do we end up with an inconsistent number of columns? Aren't we filling-in
each cell?

[saroyanm, 2018/04/26 17:53:51]

Meeting note: We will use new column called filename and add missing commas when
the file is generated.

[saroyanm, 2018/05/04 13:51:08]

Done.

[saroyanm, 2018/05/04 13:51:08]

Apparently we were generating right amount of commas.

+  }).then((dataMatrix) =>
+  {
+    let headers = dataMatrix.shift();
+    let [headId, headDescription, headPlaceholder, ...headLocales] = headers;
+    let dataTreeObj = {};
+    let currentFilename = "";
+    for(let rowId in dataMatrix)
+    {
+      let row = dataMatrix[rowId];
+      let [stringId, description, placeholder, ...messages] = row;
+      if (!stringId)
+        continue;
+
+      stringId = stringId.trim();
+      // Check if it's the filename row
+      if (stringId.endsWith(".json"))
+      {
+        currentFilename = stringId;
+        dataTreeObj[currentFilename] = {};
+        continue;
+      }
+
+      description = description.trim();
+      for (let i = 0; i < headLocales.length; i++)
+      {
+        let locale = headLocales[i].trim();
+        let message = messages[i].trim();
+        if (!message)
+          continue;
+
+        // Create Object tree from the Objects array, for easier search
+        // ex.: {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
+        if (!dataTreeObj[currentFilename][locale])
+          dataTreeObj[currentFilename][locale] = {};
+
+        let localeObj = dataTreeObj[currentFilename][locale];
+        localeObj[stringId] = {};

[Thomas Greiner, 2018/03/19 18:28:02]

Detail: You're referencing `localeObj[stringId]` a couple of times here. We can
easily avoid this by introducing a new variable for it. Thereby we don't end up
accidentally writing `localeObj` instead of `localeObj[stringId]`.

[saroyanm, 2018/04/26 17:53:52]

Acknowledged.

[saroyanm, 2018/05/04 13:51:09]

Done.

+
+        // We keep string descriptions only in default locale files
+        if (locale == defaultLocale)
+          localeObj[stringId].description = description;
+
+        localeObj[stringId].message = message;
+
+        if (placeholder)
+          localeObj[stringId].placeholders = JSON.parse(placeholder);
+      }
+    }
+    writeJson(dataTreeObj);
+  });
+}
+
+/**
+ * Write locale files according to dataTreeObj
+ * @param  {Object} dataTreeObj - ex.:
+ * {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
+ */
+function writeJson(dataTreeObj)
+{
+  for (let fileName in dataTreeObj)
+  {
+    for (let locale in dataTreeObj[fileName])

[saroyanm, 2018/02/28 20:57:34]

When writing to the file we should first find a way to sort StringIDs for the
locales(translations), because currently the translation JSON files contain
sorted stringIDs.

[Thomas Greiner, 2018/03/19 18:28:04]

We cannot rely on the order of object properties because it's not part of the
ECMAScript standard. Apart from that, why would we care about their order?

[saroyanm, 2018/04/26 17:53:52]

Meeting note: This is required, because we don't want to have ugly diffs,
because the Crowdin sorts the JSON.

We will rely currently on V8 to sort, but also add a comment and create an issue
to discuss this further.

[saroyanm, 2018/05/04 13:51:11]

Done.

+    {
+      let filePath = path.join(localesDir, locale, fileName);
+      let fileString = JSON.stringify(dataTreeObj[fileName][locale], null, 2);
+
+      // Newline at end of file to match Coding Style
+      if (locale == defaultLocale)
+        fileString += "\n";
+      fs.writeFile(filePath, fileString, "utf8", (err) =>
+        {
+          if (err)
+          {
+            console.error(err);
+          }
+          else
+          {
+            console.log(`Updated: ${filePath}`);
+          }
+        });
+    }
+  }
+}
+
+/**
+ * Convert two dimensional array to the CSV file
+ * @param  {string[][]} csvArray - array to convert from
+ */
+function arrayToCsv(csvArray)
+{
+  csv.stringify(csvArray, (err, output) =>
+  {
+    fs.writeFile(outputFileName, output, "utf8", function (err)

[Thomas Greiner, 2018/03/19 18:28:03]

Coding style: `function (err)` violates the following ESLint rules:
"prefer-arrow-callback", "space-before-function-paren"

Please make sure to run `npm run lint` to catch such issues.

[saroyanm, 2018/04/26 17:53:51]

Acknowledged.

[saroyanm, 2018/05/04 13:51:08]

Done.

+    {
+      if (!err)
+        console.log(`${outputFileName} is created`);

[Thomas Greiner, 2018/03/19 18:28:04]

We should only ignore errors in exceptional cases. At the very least we should
output them to the console so that we know when an error has occurred.

[saroyanm, 2018/04/26 17:53:51]

Acknowledged.

[saroyanm, 2018/05/04 13:51:07]

Done.

+    });
+  });
+}
+
+/**
+ * Reads JSON file and assign filename and locale to it
+ * @param  {string} locale - ex.: "en_US", "de"...
+ * @param  {string} file - ex.: "desktop-options.json" 

[Thomas Greiner, 2018/03/19 18:28:04]

Detail: Again, "file" is not what you call it in the function signature below.

See also
https://codereview.adblockplus.org/29636585/diff2/29645625:29711668/csv-expor...

[saroyanm, 2018/05/04 13:51:07]

Done.

+ * @return {Promise<Object>} fileName, locale and Strings of locale file

[Thomas Greiner, 2018/03/19 18:28:03]

Typo: Replace "Strings" with "strings"

[saroyanm, 2018/05/04 13:51:10]

Done.

+ */
+function readJson(locale, fileName)
+{
+  return new Promise((resolve, reject) =>
+  {
+    let filePath = path.join(localesDir, locale, fileName);
+    fs.readFile(filePath, (err, data) =>
+    {
+      if (err)
+      {
+        reject(err);
+      }
+      else
+      {
+        resolve({fileName, locale, strings: JSON.parse(data)});
+      }
+    });
+    // Continue Promise.All even if rejected.

[Thomas Greiner, 2018/03/19 18:28:02]

Detail: Why? Not being able to read from a JSON file sounds like a legitimate
reason to stop the export.

[saroyanm, 2018/05/04 13:51:11]

Done, beforehand I was using locales and filenames from the defaultLocale
directory, which lead to iterating and having this hack for the files that
doesn't exist, currently I use glob, in order not to read content recursively
and I think current implementation makes more sense, thanks for pointing out on
this.

+  }).catch(reason => {});
+}
+
+/**
+ * Reads file
+ * @param  {string} filePath
+ * @return {Promise<Object>} contents of file in given location
+ */
+function readFile(filePath)
+{
+  return new Promise((resolve, reject) =>
+  {
+    fs.readFile(filePath, "utf8", (err, data) =>
+    {
+      if (err)
+        reject(err);
+      else
+        resolve(data);
+    });
+  });
+}
+
+/**
+ * Read files and folder names inside of the directory
+ * @param  {string} dir - path of the folder
+ * @return {Promise<Object>} array of folders

[Thomas Greiner, 2018/03/19 18:28:03]

Detail: The return type is `Promise<string[]>`.

[Thomas Greiner, 2018/03/19 18:28:03]

Suggestion: Technically, those can be either folders or regular files so calling
them files may be more appropriate.

[saroyanm, 2018/05/04 13:51:09]

Irrelevant in the new patch.

+ */
+function readDir(dir)

[Thomas Greiner, 2018/03/19 18:28:04]

Suggestion: You could avoid having to write such functions by using Node's
built-in `promisify()` function which you're already importing anyway.

  let fsReaddir = util.promisify(fs.readdir);

Same applies to `readFile()`, `readJson()` and potentially other parts of the
code.

See also https://nodejs.org/api/util.html#util_util_promisify_original

[saroyanm, 2018/04/26 17:53:52]

Agree.

[saroyanm, 2018/05/04 13:51:07]

Done.

+{
+  return new Promise((resolve, reject) =>
+  {
+    fs.readdir(dir, (err, folders) =>
+    {
+      if (err)
+        reject(err);
+      else
+        resolve(folders);
+    });
+  });
+}
+
+/**
+ * Executing mercurial commands on the system level
+ * @param  {string} command - mercurial command ex.:"hg ..."
+ * @return {Promise<Object>} output of the command
+ */
+function executeMercurial(commands)
+{
+  return new Promise((resolve, reject) =>
+  {
+    exec(`hg ${commands.join(" ")}`, (err, output) =>

[Thomas Greiner, 2018/03/19 18:28:04]

Detail: `child_process.execFile()` already does what we want to do here. It
allows us to call a command and passing it an array of strings as arguments.

See
https://nodejs.org/api/child_process.html#child_process_child_process_execfil...

[saroyanm, 2018/05/04 13:51:08]

Done.

+    {
+      if (err)
+        reject(err);
+      else
+        resolve(output);
+    });
+  });
+}
+
+// CLI
+let helpText = `
+About: Converts locale files between CSV and JSON formats
+Usage: csv-export.js [option] [argument]
+Options:
+  -f [FILENAME]         Name of the files to be exported ex.: -f firstRun.json
+                        option can be used multiple times.
+                        If omitted all files are being exported
+
+  -o [FILENAME]         Output filename ex.:
+                        -f firstRun.json -o {hash}-firstRun.csv

[Thomas Greiner, 2018/03/19 18:28:02]

Detail: Be careful when passing arguments like that. The `{…}` expression may be
misinterpreted so it's usually best to put quotation marks around argument
values.

Also applies to the other examples.

[saroyanm, 2018/05/04 13:51:10]

Not sure I understand the comment. I'm not passing arguments?

[Thomas Greiner, 2018/05/07 15:16:33]

I'm referring to the CLI argument `-o {hash}-firstRun.csv`. For instance, you
can rename a file in UNIX via `mv {old,new}-file.txt`

+                        Placeholders:
+                          {hash} - Mercurial current revision hash
+                          {repo} - Name of the "Default" repository
+                        If omitted the output fileName is set to
+                        translations-{repo}-{hash}.csv
+
+  -i [FILENAME]         Import file path ex: -i issue-reporter.csv
+`;
+
+let arguments = process.argv.slice(2);
+let stopExportScript = false;
+// Filter to be used export to the fileNames inside
+let filesFilter = [];
+
+for (let i = 0; i < arguments.length; i++)
+{
+  switch (arguments[i])
+  {
+    case "-h":
+      console.log(helpText);
+      stopExportScript = true;
+      break;
+    case "-f":
+      // check if argument following option is specified
+      if (!arguments[i + 1])
+      {
+        process.exit("Please specify the input filename");

[Thomas Greiner, 2018/03/19 18:28:04]

This is not how you call `process.exit()`.

See https://nodejs.org/api/process.html#process_process_exit_code.

[saroyanm, 2018/05/04 13:51:10]

Done.

+      }
+      else
+      {
+        filesFilter.push(arguments[i + 1]);
+      }
+      break;
+    case "-o":
+      if (!arguments[i + 1])
+      {
+        process.exit("Please specify the output filename");
+      }
+      else
+      {
+        outputFileName = arguments[i + 1];
+      }
+      break;
+    case "-i":
+      if (!arguments[i + 1])
+      {
+        process.exit("Please specify the import file");
+      }
+      else
+      {
+        let importFile = arguments[i + 1];
+        importTranslations(importFile);
+        stopExportScript = true;
+      }
+      break;
+  }
+}
+
+if (!stopExportScript)
+  exportTranslations(filesFilter);