Issue 6171 - create CSV exporter and importer for translations

build/csv-export.js

 /*

[saroyanm, 2018/05/24 17:00:59]

In some stage of import/export This "\u00A0" special character were lost ->
https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/22/diffs#not...

This seem to be an unexpected behavior.

[Thomas Greiner, 2018/05/25 10:23:28]

Based on what I see in
https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/22/diffs?com...
it is there. Note that it's visually indistinguishable to a regular whitespace
character, similar on an HTML page when you write ` `.

You can easily check what character it is by checking its char code.

i.e.
" ".charCodeAt(0) === "\u00A0".charCodeAt(0) // code: 160
" ".charCodeAt(0) === "\u0020".charCodeAt(0) // code: 32

Therefore it doesn't make a difference for us how it shows up in the code. The
important thing is that C&T is aware of that and can ensure that translators
enter the appropriate whitespace characters.

PS: Copy-pasting the above code from Rietveld won't work because they convert
the character into a regular whitespace so make sure to use the character from
the diff instead.

[saroyanm, 2018/05/28 13:37:08]

Yes, it's there because I've added them manually.
See old diff ->
https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/22/diffs?dif...

But you are right that it was converted to actual character, in one of the stage
of import and export, rather than code point. Thanks for pointing this out. I
think in that case we don't need to do anything. I wonder why we used code point
in the first place ?

[Thomas Greiner, 2018/05/28 17:12:46]

No particular reason. It was just easier to write and see it this way.

Ideally, we would convert all whitespaces (except 0x0020) and non-printable
characters into this represenation to allow us to spot oddities during review.
But that might be difficult to achieve and it's more of a problem for C&T than
for us anyway.

  * 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 process */
 
 "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 readFile = promisify(fs.readFile);
+const writeFile = promisify(fs.writeFile);
 const glob = promisify(require("glob").glob);
-const readJsonPromised = promisify(readJson);
 
 const localesDir = "locale";
 const defaultLocale = "en_US";
 
-let headers = ["Filename", "StringID", "Description", "Placeholders",
-               defaultLocale];
+const headers = ["Filename", "StringID", "Description", "Placeholders",
+                 defaultLocale];
 let outputFileName = "translations.csv";
 
 /**
  * Export existing translation - files into CSV file
  */
 function exportTranslations()
 {
   glob(`${localesDir}/**/*.json`).then((filePaths) =>
   {
     // Reading all existing translations files
-    return Promise.all(filePaths.map((filePath) => readJsonPromised(filePath)));
+    return Promise.all(filePaths.map((filePath) => readJson(filePath)));
   }).then(csvFromJsonFileObjects);
 }
 
 /**
  * Creating Matrix which reflects output CSV file
  * @param  {Object[]} fileObjects - array of file objects created by readJson
  */
 function csvFromJsonFileObjects(fileObjects)
 {
-  let locales = [];
+  const locales = [];
+  const fileNames = [];
   // Create Object tree from the Objects array, for easier search
   // ex.: {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
-  let dataTreeObj = Object.create(null);
-  for (let fileObject of fileObjects)
+  const dataTreeObj = Object.create(null);
+  for (const fileObject of fileObjects)
   {
     const {fileName, locale, strings} = fileObject;
 
-    if (!locales.includes(locale))
+    if (locale != defaultLocale && !locales.includes(locale))
       locales.push(locale);
 
-    if (!dataTreeObj[fileName])
-      dataTreeObj[fileName] = {};
-    if (!dataTreeObj[fileName][locale])
-      dataTreeObj[fileName][locale] = {};
+    if ((!filesFilter.length || filesFilter.includes(fileName)) &&
+         !fileNames.includes(fileName))
+      fileNames.push(fileName);
+
+    if (!(fileName in dataTreeObj))
+      dataTreeObj[fileName] = Object.create(null);
+
     dataTreeObj[fileName][locale] = strings;
   }
-
-  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/06/12 14:50:41]

Is there no need for sorting locales anymore?

[saroyanm, 2018/06/12 15:22:49]

If you mean default strings:
See -> https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/104

   // 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]);
+  const csvArray = [headers.concat(locales)];
+  for (const fileName of fileNames)
+  {
+    const strings = dataTreeObj[fileName][defaultLocale];
+    for (const stringID of Object.keys(strings))
+    {
+      const fileObj = dataTreeObj[fileName];
+      const {description, message, placeholders} = strings[stringID];
+      const row = [fileName, stringID, description || "",
+                   JSON.stringify(placeholders), message];
+
+      for (const locale of locales)
+      {
+        const localeFileObj = fileObj[locale];
+        const 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);

[saroyanm, 2018/05/24 16:40:02]

I think there might a bug in csvParser ->
https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/22/#note_748...
this will need a closer look, I don't remember this previous time when I was
importing the strings.

Note: I noticed this thanks to unsorted source JSON files.

   }).then((dataMatrix) =>
   {
-    let headLocales = dataMatrix.shift().slice(4);
-    let dataTreeObj = {};
-    for (let rowId in dataMatrix)
-    {
-      let row = dataMatrix[rowId];
+    const headLocales = dataMatrix.shift().slice(4);
+    const dataTreeObj = {};
+    for (const rowId in dataMatrix)
+    {
+      const 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();
+        const locale = headLocales[i].trim();
+        const 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];
+        const localeObj = dataTreeObj[currentFilename][locale];
         localeObj[stringId] = {};
-        let stringObj = localeObj[stringId];
+        const stringObj = localeObj[stringId];
 
         // We keep string descriptions only in default locale files
-        if (locale == defaultLocale)

[saroyanm, 2018/05/24 13:29:26]

I've changed this into if (locale == defaultLocale && description) for the lates
release in order to exclude irrelevant changes, because the generated diff were
providing additional overhead for the reviewer.

I'm not yet sure whether we need the empty description to be imported, we can I
think run the script to update the files accordingly when we have agreement on
this.

[Thomas Greiner, 2018/05/24 13:49:42]

I think your suggested solution (i.e. only include the "description" field when
there's a description that's not empty) makes the most sense.

+        if (locale == defaultLocale && description)
           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 sortedJSON = orderJSON(dataTreeObj[fileName][locale]);

[saroyanm, 2018/05/24 15:29:54]

Sorting the Default locale produce an unreadable diff, the problem is that, the
translation files are sorted when imported from the Crowdin, but the source
Languages we might don't want to sort, or tackle that later on. Currently I have
special case here that only sort the non defaultLocale strings. I will include
that in the new patch.

[Thomas Greiner, 2018/05/24 15:46:46]

I understand. In that case let's sort the default locale manually in a separate
commit. I'll leave it up to you when you prefer to do that:

- Before importing translations
  Pro: We can use this code as is.
  Con: We can't start the translations review before those changes were made.

- After importing translations
  Pro: We can start the translations review now.
  Con: We need to make the suggested change now and remove it again later.

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

I've created a gitlab issue in order to discuss this ->
https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/104

-      let fileString = JSON.stringify(sortedJSON, null, 2);
+  for (const fileName in dataTreeObj)
+  {
+    for (const locale in dataTreeObj[fileName])
+    {
+      const filePath = path.join(localesDir, locale, fileName);
+      const sortedJson = sortJson(dataTreeObj[fileName][locale]);
+      let fileString = JSON.stringify(sortedJson, 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}`);
-        }
+      writeFile(filePath, fileString, "utf8").then(() =>
+      {
+        console.log(`Updated: ${filePath}`); // eslint-disable-line no-console
+      }).catch((err) =>
+      {
+        console.error(err);
       });
     }
   }
 }
 
 /**
  * This function currently relies on V8 to sort the object by keys
  * @param {Object} unordered - json object
  * @returns {Object}
  */
-function orderJSON(unordered)
+function sortJson(unordered)
 {
   const ordered = {};
-  for (let key of Object.keys(unordered).sort())
+  for (const key of Object.keys(unordered).sort())
   {
     ordered[key] = unordered[key];
     if (unordered[key].placeholders)
-      ordered[key].placeholders = orderJSON(unordered[key].placeholders);
+      ordered[key].placeholders = sortJson(unordered[key].placeholders);
 
     ordered[key] = unordered[key];
   }
   return ordered;
 }
 
 /**
  * Convert two dimensional array to the CSV file
  * @param  {Object[]} 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);
+    writeFile(outputFileName, output, "utf8").then(() =>
+    {
+      // eslint-disable-next-line no-console
+      console.log(`${outputFileName} is created`);
+    }).catch((error) =>
+    {
+      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

[Thomas Greiner, 2018/06/12 14:50:42]

Detail: This parameter no longer exists. Instead, you can change the return type
to `Promise<Object>` and describe its contents there.

[saroyanm, 2018/06/13 15:36:25]

Done.

  *                   Parameters:
  *                     * Error message
  *                     * Object containing fileName, locale and strings
+ * @returns {Promise}
  */
 function readJson(filePath, callback)
 {
-  let {dir, base} = path.parse(filePath);
-  fs.readFile(filePath, "utf8", (err, data) =>
-  {
-    if (err)
-    {
-      callback(err);
-    }
-    else
-    {
-      let locale = dir.split(path.sep).pop();
-      let strings = JSON.parse(data);
-      callback(null, {fileName: base, locale, strings});
-    }
+  return new Promise((resolve, reject) =>

[Thomas Greiner, 2018/06/12 14:50:42]

You can simply return the promise you get from `readFile()` instead of creating
a new one. That way you won't have to worry about error handling in here.

But note that you'll need to move `path.parse()` inside the arrow function for
any of its errors to be passed on.

[saroyanm, 2018/06/13 15:36:25]

Done.

+  {
+    const {dir, base} = path.parse(filePath);
+    readFile(filePath, "utf8").then((data) =>
+    {
+      const locale = dir.split(path.sep).pop();
+      const strings = JSON.parse(data);
+      resolve({fileName: base, locale, strings});
+    }).catch((err) =>
+    {
+      reject(err);
+    });
   });
 }
 
 /**
  * Exit process and log error message
  * @param {String} error error message
  */
 function exitProcess(error)
 {
   console.error(error);
   process.exit(1);
 }
 
 // CLI
-let helpText = `
+const 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

[Thomas Greiner, 2018/06/12 14:50:42]

Detail: Again, these placeholders no longer exists so we need to update this
entire argument description.

See
https://codereview.adblockplus.org/29636585/diff2/29784598:29799613/build/csv...

[saroyanm, 2018/06/13 15:36:26]

Done.

-                          {repo} - Name of the "Default" repository

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

Detail: These placeholders no longer exist.

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

Done.

                         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);
+const argv = process.argv.slice(2);
 let stopExportScript = false;
 // Filter to be used export to the fileNames inside
-let filesFilter = [];
+const filesFilter = [];
 
 for (let i = 0; i < argv.length; i++)
 {
   switch (argv[i])
   {
     case "-h":
-      console.log(helpText);
+      console.log(helpText); // eslint-disable-line no-console
       stopExportScript = true;
       break;
     case "-f":
       if (!argv[i + 1])
       {
         exitProcess("Please specify the input filename");
       }
       filesFilter.push(argv[i + 1]);
       break;
     case "-o":
       if (!argv[i + 1])
       {
         exitProcess("Please specify the output filename");
       }
       outputFileName = argv[i + 1];
       break;
     case "-i":
       if (!argv[i + 1])
       {
         exitProcess("Please specify the import file");
       }
-      let importFile = argv[i + 1];
+      const importFile = argv[i + 1];
       importTranslations(importFile);
       stopExportScript = true;
       break;
   }
 }
 
 if (!stopExportScript)
   exportTranslations(filesFilter);