Issue 6171 - create CSV exporter and importer for translations

csv-export.js

Index: csv-export.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/csv-export.js
@@ -0,0 +1,356 @@
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * 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/>.
+ */
+
+/* globals require, process */

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

Detail: No need to list `require` as a global anymore because we added it
globally in
https://gitlab.com/eyeo/adblockplus/adblockplusui/commit/08a2c66736a1f489fe76...

[saroyanm, 2018/05/16 17:06:34]

Done.

+
+"use strict";
+
+const fs = require("fs");
+const path = require("path");
+const csv = require("csv");
+const {promisify} = require("util");
+const execFile = promisify(require("child_process").execFile);
+const csvParser = promisify(csv.parse);
+const readDir = promisify(fs.readdir);

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

This variable is unused.

[saroyanm, 2018/05/16 17:06:32]

Done.

+const readFile = promisify(fs.readFile);

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

This variable is unused so let's use it instead of `fs.readFile()`.

[saroyanm, 2018/05/16 17:06:33]

The variable is used. see, line: 131

[Thomas Greiner, 2018/05/22 17:22:48]

Still, you're using `fs.readFile()` in `readJson()`.

[saroyanm, 2018/06/05 15:03:45]

Done.

+const glob = promisify(require("glob").glob);
+const readJsonPromised = promisify(readJson);

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

Why do you call `promisify()` on your own function? You can just convert it to
return a promise.

[saroyanm, 2018/05/15 18:59:48]

Because of current comment ->
https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco...


I thought you want to use promisify to simplify the Promise implementation.

[Thomas Greiner, 2018/05/22 17:22:47]

That's because we cannot change how native Node functions are implemented so all
we can do is convert the callback into a promise using `promisify()`.

However, for functions that we create ourselves we don't need to implement them
using callbacks but can return the promise directly and thereby don't need this
conversion step.

+
+const localesDir = "locale";
+const defaultLocale = "en_US";
+
+let headers = ["filename", "StringID", "Description", "Placeholders",

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

Detail: Unlike the other headers, "filename" doesn't start with a capital
character.

[saroyanm, 2018/05/16 17:06:33]

Done.

+               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(execFile("hg", ["id", "-i"]));
+  // Get repo path
+  mercurialCommands.push(execFile("hg", ["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.stdout.replace(/\+\n|\n$/, ""));

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

Suggestion: You're calling the array "outputs" but the items are called "item"
so why not call them "output" to make it less generic?

[saroyanm, 2018/05/16 17:06:31]

Done.

+    // 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 glob(`${localesDir}/**/*.json`, {});

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

Detail: The second argument is optional according to
https://github.com/isaacs/node-glob#globpattern-options-cb.

[saroyanm, 2018/05/16 17:06:32]

Done.

+  }).then((filePaths) =>
+  {
+    // Reading all existing translations files
+    return Promise.all(filePaths.map((filePath) => readJsonPromised(filePath)));
+  }).then(csvFromJsonFileObjects);
+}
+
+/**
+ * Creating Matrix which reflects output CSV file
+ * @param  {Array} fileObjects - array of file objects created by readJson

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

Detail: Arrays of objects are described as `Object[]`.

Note that `Array<Object>` (or more accurately `Array.<Object>`) would be another
option but that'd be inconsistent which how we describe arrays throughout our
code.

[saroyanm, 2018/05/16 17:06:34]

Done.

+ */
+function csvFromJsonFileObjects(fileObjects)
+{
+  let locales = [];
+  // Create Object tree from the Objects array, for easier search
+  // ex.: {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
+  let dataTreeObj = fileObjects.reduce((accumulator, fileObject) =>

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

This `reduce()` is no longer just populating a single object (i.e.
`dataTreeObj`) but also `locales` and `fileNames`. In that regard a regular loop
seems more appropriate to avoid confusion.

You could then even split this off into its own function if you want.

e.g.

let dataTree = Object.create(null);
let fileNames = [];
let locales = [];

for (let fileObject of fileObjects)
{
  if (!fileObject)
    continue;

  let {fileName, locale, strings} = fileObject;

  if (locale != defaultLocale && !locales.include(locale))
  {
    locales.push(locale);
  }

  if (!filesFilter.length || filesFilter.includes(fileName))
  {
    fileNames.push(fileName);
  }

  if (!(fileName in dataTree))
  {
    dataTree[fileName] = Object.create(null);
  }
  dataTree[fileName][locale] = strings;
}

locales.sort();

[saroyanm, 2018/05/16 17:06:32]

Done.

[Thomas Greiner, 2018/05/22 17:22:48]

You replaced the `reduce()` call with a for-of loop now. It still contains the
same logic as before though, which means that there's still redundant loops for
populating the `locales` and `fileNames` arrays.

[saroyanm, 2018/06/04 15:17:59]

I don't understand this comment, what is wrong with current implementation ?

[Thomas Greiner, 2018/06/04 16:58:21]

The problem I'm referring to is that you're handling each locale and each file
name twice: Once in this loop and the second time in `locales.filter()` and
`fileNames.filter()` respectively.

Consider this question: Why would you add an element to a list that you're going
to filter out right afterwards anyway?

So instead of filtering elements after you add them, you could simply add a
check in this loop before you add them (see code example in original comment).

[saroyanm, 2018/06/05 10:35:26]

Got it, thanks for the clarification, I'll fix that.

[saroyanm, 2018/06/05 15:03:44]

Done.

+  {
+    if (!fileObject)
+      return accumulator;
+
+    let {fileName, locale} = fileObject;
+    if (!locales.includes(locale))
+      locales.push(locale);
+
+    if (!accumulator[fileName])
+    {
+      accumulator[fileName] = {};
+    }
+    accumulator[fileName][locale] = fileObject.strings;
+    return accumulator;
+  }, {});
+
+  let fileNames = Object.keys(dataTreeObj);
+  if (filesFilter.length)
+    fileNames = fileNames.filter((item) => filesFilter.includes(item));
+
+  locales = locales.filter((locale) => locale != defaultLocale).sort();

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

Why do you sort the locales? That step didn't exist in the previous revision so
I'm wondering whether it is necessary.

[saroyanm, 2018/05/16 17:06:33]

I think I did that to have consistent CSV file generated with ordered locales.

[Thomas Greiner, 2018/05/22 17:22:49]

Acknowledged.

+  // Create two dimensional strings array that reflects CSV structure
+  let csvArray = [headers.concat(locales)];
+  for (let fileName of fileNames)
+  {
+    let strings = dataTreeObj[fileName][defaultLocale];
+    for (let stringID of Object.keys(strings))
+    {
+      let fileObj = dataTreeObj[fileName];
+      let {description, message, placeholders} = strings[stringID];
+      let row = [fileName, stringID, description || "",
+                 JSON.stringify(placeholders), message];
+
+      for (let locale of locales)
+      {
+        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, "utf8").then((fileObjects) =>
+  {
+    return csvParser(fileObjects);
+  }).then((dataMatrix) =>
+  {
+    let headLocales = dataMatrix.shift().slice(4);
+    let dataTreeObj = {};
+    for (let rowId in dataMatrix)
+    {
+      let row = dataMatrix[rowId];
+      let [currentFilename, stringId, description, placeholder, ...messages] =
+        row;
+      if (!stringId)
+        continue;
+
+      stringId = stringId.trim();
+      // Check if it's the filename row
+      if (!dataTreeObj[currentFilename])
+        dataTreeObj[currentFilename] = {};
+
+      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] = {};
+        let stringObj = localeObj[stringId];
+
+        // We keep string descriptions only in default locale files
+        if (locale == defaultLocale)
+          stringObj.description = description;
+
+        stringObj.message = message;
+        if (placeholder)
+          stringObj.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])
+    {
+      let filePath = path.join(localesDir, locale, fileName);
+      let orderedJSON = orderJSON(dataTreeObj[fileName][locale]);

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

Suggestion: Usually, we'd use the word "sort" instead of "order". A good reason
I could think of is because "to order" can also mean "to make an order" or "to
command".

[saroyanm, 2018/05/16 17:06:31]

Done.

[Thomas Greiner, 2018/05/22 17:22:47]

Note that while you renamed the variable, the function is still called
"orderJSON".

Also note the inconsistency in the capitalization.

i.e.
- csvFromJsonFileObjects
- orderJSON
- orderedJSON
- readJson
- writeJson

It'd be great if we could agree on a naming convention for such cases.

[saroyanm, 2018/06/05 15:03:44]

Done.

+      let fileString = JSON.stringify(orderedJSON, 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}`);
+        }
+      });
+    }
+  }
+}
+
+/**
+ * This function currently rely on nodeJS to sort the object by keys

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

Detail: Technically, it relies on V8's implementation, not Node's.

However, I found
https://github.com/nodejs/node/issues/15628#issuecomment-332588533 which states
that in ES2016 the order is now deterministic based on the order in which
properties got added.
Therefore we could remove this comment and instead simply describe what this
function is for.

Alternatively, we could use https://github.com/substack/json-stable-stringify
which was suggested in the comment I linked to above.

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

Typo: Replace "rely" with "relies".

[saroyanm, 2018/05/16 17:06:34]

Done.

[saroyanm, 2018/05/16 17:06:35]

Done
While current version works reliable enough, I suggest we discuss better
implementation in the separate ticket.
Let's go with the "Detail" fix for now.

[Thomas Greiner, 2018/05/22 17:22:48]

Acknowledged.

+ * @param {Object} unordered - json object
+ * @returns {Object}
+ */
+function orderJSON(unordered)
+{
+  const ordered = {};
+  for (let key of Object.keys(unordered).sort())
+  {
+    ordered[key] = unordered[key];
+    if (unordered[key].placeholders)
+      ordered[key].placeholders = orderJSON(unordered[key].placeholders);
+
+    ordered[key] = unordered[key];
+  }
+  return ordered;
+}
+
+/**
+ * Convert two dimensional array to the CSV file
+ * @param  {Array} csvArray - array to convert from
+ */
+function arrayToCsv(csvArray)
+{
+  csv.stringify(csvArray, (err, output) =>
+  {
+    fs.writeFile(outputFileName, output, "utf8", (error) =>
+    {
+      if (!error)
+        console.log(`${outputFileName} is created`);
+      else
+        console.error(error);
+    });
+  });
+}
+
+/**
+ * Reads JSON file and assign filename and locale to it
+ * @param {string} filePath - ex.: "locales/en_US/desktop-options.json"
+ * @param {function} callback - fileName, locale and strings of locale file
+ *                   Parameters:
+ *                     * Error message
+ *                     * Object containing fileName, locale and strings
+ */
+function readJson(filePath, callback)
+{
+  let {dir, base} = path.parse(filePath);
+  fs.readFile(filePath, "utf8", (err, data) =>
+  {
+    if (err)
+    {
+      callback(err);
+    }
+    else
+    {
+      callback(null, {fileName: base, locale: dir.split("/").pop(),

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

The path delimiter is OS-specific so please use `path.delimiter` instead.

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

Detail: Let's avoid doing multiple things in a single statement to make it
easier to maintain.

i.e.
let locale = dir.split("/").pop();
let strings = JSON.parse(data);
callback(null, {fileName: base, locale, strings}), 

[saroyanm, 2018/05/16 17:06:31]

Done.

[saroyanm, 2018/05/16 17:06:32]

Done. I think you meant `path.sep`

[Thomas Greiner, 2018/05/22 17:22:47]

Yep, you're right.

+        strings: JSON.parse(data)});
+    }
+  });
+}
+
+/**
+ * Exit process and log error message
+ * @param {String} error error message
+ */
+function exitProcess(error)
+{
+  console.error(error);
+  process.exit();

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

We should pass a number that's not `0` when exiting the process with an error.
That way other CLI programs who call our script know whether the import/export
failed.

Passing `1` would be sufficient for now.

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

[saroyanm, 2018/05/16 17:06:31]

OOps, thanks, done.

+}
+
+// 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
+                        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 argv = process.argv.slice(2);
+let stopExportScript = false;
+// Filter to be used export to the fileNames inside
+let filesFilter = [];
+
+for (let i = 0; i < argv.length; i++)
+{
+  switch (argv[i])
+  {
+    case "-h":
+      console.log(helpText);
+      stopExportScript = true;

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

Suggestion: What about moving the argument processing logic into its own
function which returns this boolean? That way you could just do `return true`
here and avoid the global `stopExportScript` variable.

[saroyanm, 2018/05/16 17:06:35]

As was mentioned here ->
https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js...


This logic going change anyway, so I think best will be to tackle that
separately.
Please let me know if you want me to do that changes here.

[Thomas Greiner, 2018/05/22 17:22:48]

Ok.

+      break;
+    case "-f":
+      // check if argument following option is specified

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

Detail: The code seems self-explanatory so maybe best to remove this comment. At
least it's not used for the other occurrences.

[saroyanm, 2018/05/16 17:06:35]

Done.

+      if (!argv[i + 1])
+      {
+        exitProcess("Please specify the input filename");
+      }
+      else

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

Detail: This else-statement is redundant because we're exiting in the
if-statement.

[saroyanm, 2018/05/16 17:06:34]

Done.

+      {
+        filesFilter.push(argv[i + 1]);
+      }
+      break;
+    case "-o":
+      if (!argv[i + 1])
+      {
+        exitProcess("Please specify the output filename");
+      }
+      else

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

Detail: This else-statement is redundant because we're exiting in the
if-statement.

[saroyanm, 2018/05/16 17:06:33]

Done.

+      {
+        outputFileName = argv[i + 1];
+      }
+      break;
+    case "-i":
+      if (!argv[i + 1])
+      {
+        exitProcess("Please specify the import file");
+      }
+      else

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

Detail: This else-statement is redundant because we're exiting in the
if-statement.

[saroyanm, 2018/05/16 17:06:33]

Done.

+      {
+        let importFile = argv[i + 1];
+        importTranslations(importFile);
+        stopExportScript = true;
+      }
+      break;
+  }
+}
+
+if (!stopExportScript)
+  exportTranslations(filesFilter);