Issue 6171 - create CSV exporter and importer for translations

csv-export.js

 /*

[Thomas Greiner, 2018/01/22 19:49:52]

While we're waiting for the final results from the tooling discussion, we can
already introduce a package.json and move this script into a build directory
since we've already agreed on using Node.

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

I'll address this in a separate patch. Moving this to the buildtools directory
causes an issue after running "ensure_dependencies.py".

  * 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";
 
-let filesNames = []; // ex.: desktop-options.json

[Thomas Greiner, 2018/01/22 19:49:52]

Detail: Usually you'd call this variable "fileNames" or "filenames" the way you
have a "fileObjects" variable.

The reason why I'm a bit nit-picky about this name is that it's easy to
accidentally introduce two separate variables "fileNames" and "filesNames" by
accident which would be tricky to spot.

[saroyanm, 2018/02/28 20:48:26]

Done.

-let locales = [];  // List of all available locale codes

[Thomas Greiner, 2018/01/22 19:49:53]

Detail: We tend to put comments in their own line to make them easier to spot if
you don't have syntax highlighting enabled.

[saroyanm, 2018/02/28 20:48:25]

Done.

+// 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";

[Thomas Greiner, 2018/01/22 19:49:53]

Detail: This custom template seems redundant since we can use template literals
when constructing the file name.

[saroyanm, 2018/02/28 20:48:22]

We can't pass Template literals using CLI, until I missunderstood your question.

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

This is not a template literal.

What I meant is to:
1. Set this line to `let outputFileName;`
2. Replace line 54 and 55 with `outputFileName =
`translations-${repo}-${hash}.csv`;`

But I see that this string depends on the `-o` argument's value. So the question
is whether we need to support custom output file names.

 
 /**
- * Export existing translation files into CSV file
- * @param  {[type]} filesFilter Optional parameter which allow include only

[Thomas Greiner, 2018/01/22 19:49:49]

Suggestion: What I tend to do to visually differentiate the parameter name from
its description is to separate them with a "-" character.

e.g.
@param {boolean} isHidden - Indicates whether element should be hidden

[Thomas Greiner, 2018/01/22 19:49:50]

Detail: No need to include the text "Optional" because the brackets already
indicate that.

Note, however, that the brackets are supposed to go around the parameter name,
not the type (see
http://usejsdoc.org/tags-param.html#optional-parameters-and-default-values).

[Thomas Greiner, 2018/01/22 19:49:51]

Detail: "[type]" is not a valid type.

Also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:24]

Right, it's the default value generated by DocBlockr.
Done.

[saroyanm, 2018/02/28 20:48:27]

Done.

[saroyanm, 2018/02/28 20:48:31]

Done.

- *                              fileNames in the array, if ommited all files

[Thomas Greiner, 2018/01/22 19:49:51]

Typo: Replace "ommited" with "omitted"

Also applies to other occurrences in this file.

[saroyanm, 2018/02/28 20:48:30]

Done.

- *                              will be exported
+ * Export existing translation - files into CSV file
+ * @param  {string[]} [filesFilter] - fileNames filter, if omitted all files 
+ *                                    will be exported
  */
 function exportTranslations(filesFilter)
 {
   let mercurialCommands = [];
-  mercurialCommands.push(executeMercurial("hg id -i")); // Get Hash
-  mercurialCommands.push(executeMercurial("hg paths default")); // Get repo path
+  // Get Hash
+  mercurialCommands.push(executeMercurial(["id", "-i"]));
+  // Get repo path
+  mercurialCommands.push(executeMercurial(["paths", "default"]));
   Promise.all(mercurialCommands).then((outputs) =>

[Thomas Greiner, 2018/01/22 19:49:50]

You've nicely split up each step in the process into a different arrow function
in the promise chain. But why cram them all together inside
`exportTranslations()` when you could turn them into separate functions and just
call them in here?

That way the code would be easier to read and understand.

[saroyanm, 2018/02/28 20:48:23]

I have simplified this function, can try to simplify even more with next patch.

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

That'd be great, thanks. The more we can disconnect functions from each other
the easier it should be to maintain the code.

   {
     // Remove line endings and "+" sign from the end of the hash

[Thomas Greiner, 2018/01/22 19:49:49]

Mercurial revision IDs are written in hexadecimal so there shouldn't be any "+"
characters in there.

See also https://www.mercurial-scm.org/wiki/Nodeid

[saroyanm, 2018/02/28 20:48:32]

I do use "hg id -i" on my local machine and it returns "+" sign in the end.
Can you please confirm that you are having a different result ?

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

According to `hg id --help`:

    Print a summary identifying the repository state at REV using one or two
    parent hash identifiers, followed by a "+" if the working directory has
    uncommitted changes, the branch name (if not default), a list of tags, and
    a list of bookmarks.

So it sounds like we should keep the "+" so that we can differentiate between
the actual revision and local changes that are based on the revision.

-    let [hash, path] = outputs.map((item) => item.replace(/\+\n|\n$/, ""));

[Thomas Greiner, 2018/01/22 19:49:52]

Detail: Seems like you'd want to use `item.trim()` instead to simplify this
code.

[saroyanm, 2018/02/28 20:48:24]

If not the "+" sign the trim would have worked here probably.

-    // Update name of the file to be outputted

[Thomas Greiner, 2018/01/22 19:49:54]

Typo: Replace "outputted" with "output"

[saroyanm, 2018/02/28 20:48:24]

Done.

+    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.split("/").pop());

[Thomas Greiner, 2018/01/22 19:49:48]

This code is OS-specific so let's instead use Node's path module to determine
the file name (see
https://nodejs.org/api/path.html#path_path_basename_path_ext).

[saroyanm, 2018/02/28 20:48:24]

Done.

-
-    // Prepare to read all available locales and default files  
-    let readDirectories = [];

[Thomas Greiner, 2018/01/22 19:49:51]

Suggestion: You can avoid this temporary variable if you want by writing this
as:

return Promise.all([
  readDir(…),
  readDir(…)
]);

[saroyanm, 2018/02/28 20:48:28]

Done.

-    readDirectories.push(readDir(`${localesDir}/${defaultLocale}`));

[Thomas Greiner, 2018/01/22 19:49:52]

This code is OS-specific so let's instead use Node's path module to construct
paths (see https://nodejs.org/api/path.html#path_path_join_paths).

This also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:23]

Done.

-    readDirectories.push(readDir(localesDir));
-    return Promise.all(readDirectories);
+    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) =>

[Thomas Greiner, 2018/01/22 19:49:48]

Detail: This variable is redundant because you could directly write this as
`.then(([fileNames, locales]) =>`

[saroyanm, 2018/02/28 20:48:32]

I think I can't as I assigning the variables to the ones declared in the Global
scope.

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

Then you should use a different name for one of the two variables. :)

Actually, why is `fileNames` a global variable in the first place? You only
access it here and in the `csvFromJsonFileObjects()` function so you can simply
pass it along to that function.
The same applies to `locales`.

   {
-    [filesNames, locales] = files;
+    [fileNames, locales] = files;
     // Filter files
     if (filesFilter.length)
-      filesNames = filesNames.filter((item) => filesFilter.includes(item));
+      fileNames = fileNames.filter((item) => filesFilter.includes(item));
 
     let readJsonPromises = [];
-    for(let file of filesNames)

[Thomas Greiner, 2018/01/22 19:49:50]

Coding style: Technically, this is not a violation but we have previously agreed
on using braces for multi-line blocks (see
https://issues.adblockplus.org/ticket/3692#comment:50).

[saroyanm, 2018/02/28 20:48:31]

Done.

+    for(let fileName of fileNames)
+    {
       for(let locale of locales)
-        readJsonPromises.push(readJson(locale, file));
+      {
+        readJsonPromises.push(readJson(locale, fileName));
+      }
+    }
 
     // Reading all existing translations files
     return Promise.all(readJsonPromises);
-  }).then((fileObjects) =>
-  {
-    // Create Object tree from the Objects array, for easier search
-    // ex.: {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
-    let dataTreeObj = fileObjects.reduce((acc, fileObject) =>

[Thomas Greiner, 2018/01/22 19:49:54]

Detail: Please use descriptive variable names because while I assume that "acc"
is short for "accumulated" it is not clear.

[saroyanm, 2018/02/28 20:48:29]

Done.

-    {
-      if (!fileObject)
-        return acc;
-
-      let filename = fileObject.filename;
-      let locale = fileObject.locale;

[Thomas Greiner, 2018/01/22 19:49:50]

Detail: This becomes a bit simpler when using destructuring:

let {filename, locale} = fileObject;

[saroyanm, 2018/02/28 20:48:31]

Done.

-      if (!acc[filename])
-      {
-        acc[filename] = {};
-      }
-      acc[filename][locale] = fileObject.strings;
-      return acc;
-    }, {});
-
-    // Create two dimentional strings array that reflects CSV structure

[Thomas Greiner, 2018/01/22 19:49:49]

Typo: Replace "dimentional" with "dimensional"

Also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:25]

Done.

-    let localesWithoutDefault = locales.filter((item) => item != defaultLocale);

[Thomas Greiner, 2018/01/22 19:49:54]

Detail: Why do you call it "item" here when you know that it's a locale? At
least you call the array it's in "locales" and the value you compare it to
"defaultLocale" so it seems inconsistent.

[saroyanm, 2018/02/28 20:48:31]

Done.

-    let csvArray = [headers.concat(localesWithoutDefault)];
-    for (let file of filesNames)

[Thomas Greiner, 2018/01/22 19:49:53]

Detail: "file" is a bit ambiguous since it could refer to a file path, a file
name, a file pointer, etc. so why not just stick to "fileName" based on the name
of the array it's in?

[saroyanm, 2018/02/28 20:48:23]

Done.

-    {
-      csvArray.push([file]);
-      for (let stringID in dataTreeObj[file][defaultLocale])
-      {
-        let fileObj = dataTreeObj[file];
-        let stringObj = fileObj[defaultLocale][stringID];
-        let {description, message, placeholders} = stringObj;
-
-        // Use yaml-like format for easy extraction, rather sensitive char hacks

[Thomas Greiner, 2018/01/22 19:49:54]

Instead of going with a custom syntax, we could simply serialize the whole
object as JSON instead. If we really need an easier-to-read text format, we may
find a Node module for that but first let's investigate whether JSON is
sufficient to avoid any unnecessary work.

[saroyanm, 2018/02/28 20:48:32]

Done.

-        let yamlPlaceholder = "";
-        for (let placeholder in placeholders)
-        {
-          yamlPlaceholder += `${placeholder}:\n`;
-          let {content, example} = placeholders[placeholder];
-          yamlPlaceholder += `  content: ${content}\n`;
-          yamlPlaceholder += `  example: ${example}\n`;
-        }
-
-        let row = [stringID, description || "", yamlPlaceholder, message];
-        for (let locale of localesWithoutDefault)
-        {
-          let localeFileObj = fileObj[locale];
-          let isTranslated = localeFileObj && localeFileObj[stringID];

[Thomas Greiner, 2018/01/22 19:49:53]

Detail: The name "isTranslated" implies that its value is a boolean. However,
the value of this is instead either truthy or falsey. Therefore I'd recommend
explicitly converting it into a boolean (e.g. by using `!!(…)`) to avoid any
issues this ambiguity may cause.

[saroyanm, 2018/02/28 20:48:26]

Done.

-          row.push(isTranslated ? localeFileObj[stringID].message : "");
-        }
-        csvArray.push(row);
-      }
-    }
-    arrayToCsv(csvArray); // Convert matrix to CSV
-  });
+  }).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  {[type]} filePath CSV file path to import from
+ * @param  {string} filePath - CSV file path to import from
  */
 function importTranslations(filePath)
 {
-  readCsv(filePath).then((fileObjects) =>
-  {
-    let dataMatrix = csvToArray(fileObjects);

[Thomas Greiner, 2018/01/22 19:49:51]

Let's investigate whether there's a Node module already that does that for us
and maybe even assigns headers to values so that we can easily do something
along the lines of:

readCSV(str)
  .each(({stringId, description, placeholder, ...messages}) =>
  {
    …
    for (let locale in messages)
    {
      …
      dataTreeObj[currentFilename][locale][stringId] = {
       description, placeholders,
       message: messages[locale]
      };
    }
  });

Otherwise, we may want to turn `csvToArray()` into such a function that provides
us such a simple interface.

[saroyanm, 2018/02/28 20:48:26]

Done, with finding csvToArray module. Haven't thought yet about optimizing this
implementation, let's keep it for next patch.

-    let headers = dataMatrix.splice(0, 1)[0];

[Thomas Greiner, 2018/01/22 19:49:51]

Detail: This is equivalent to `dataMatrix.shift()` so let's keep it simple.

[saroyanm, 2018/02/28 20:48:30]

Done.

+  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] = row;
+      let [stringId, description, placeholder, ...messages] = row;
       if (!stringId)
         continue;
 
       stringId = stringId.trim();
-      if (stringId.endsWith(".json")) // Check if it's the filename row

[Thomas Greiner, 2018/01/22 19:49:52]

A note for later: We probably want to retrieve the string IDs and descriptions
from the original file rather than just copying whatever is defined in the CSV
since that's coming from a third party and may not match up with what we expect
or may even contain invalid characters that could break the extension.

[saroyanm, 2018/02/28 20:48:22]

I agree, but you are right let's first address all existing issues
comments/issues and give a thought about this again.

+      // Check if it's the filename row
+      if (stringId.endsWith(".json"))
       {
         currentFilename = stringId;
         dataTreeObj[currentFilename] = {};
         continue;
       }
 
       description = description.trim();
-      placeholder = placeholder.trim();
-      for (let i = 3; i < headers.length; i++)

[Thomas Greiner, 2018/01/22 19:49:50]

This value depends on how many columns precede the locales. It is therefore
subject to change and also not explained in the code which could cause issues
later on.

Instead, we could establish an array of messages (e.g. `let [stringId,
description, placeholder, ...messages] = row;`) and deduce this value by
evaluating `row.length - messages.length` or turn the logic on its head and
writing it as:

let localeHeaderIdx = row.length - messages.length;
for (message of messages)
{
  let locale = headers[localeHeaderIdx++].trim()
  message = message.trim();
  …

[saroyanm, 2018/02/28 20:48:25]

Done.

-      {
-        let locale = headers[i].trim();
-        let message = row[i].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) =>
         {
-          let placeholders = placeholder.split("\n");
-          let placeholderName = "";
-          localeObj[stringId].placeholders = placeholders.reduce((acc, item) =>
+          if (err)
           {
-            /*
-             Placeholders use YAML like syntax in CSV files, ex:
-               tracking:
-                content: $1
-                example: Block additional tracking
-              acceptableAds:
-                content: $2
-                example: Allow Acceptable Ads
-            */
-            if (item.startsWith("  "))
-            {
-              let [key, value] = item.trim().split(":");
-              acc[placeholderName][key] = value.trim();
-            }
-            else
-            {
-              placeholderName = item.trim().replace(":", "");
-              acc[placeholderName] = {};
-            }
-            return acc;
-          }, {});
-        }
-      }
-    }
-    writeJson(dataTreeObj);
-  });
-}
-
-/**
- * Write locale files according to dataTreeObj which look like:

[Thomas Greiner, 2018/01/22 19:49:49]

Detail: There's a redundant "which look like:" in this sentence.

[saroyanm, 2018/02/28 20:48:30]

Done.

- * @param  {Object} dataTreeObj which look like:
- * {dektop-options.json: {en_US: {...}, {de: {...}, {ru: {...}}}
- */
-function writeJson(dataTreeObj)
-{
-  for (let filename in dataTreeObj)
-  {
-    for (let locale in dataTreeObj[filename])
-    {
-      let path = `${localesDir}/${locale}/${filename}`;
-      let fileString = JSON.stringify(dataTreeObj[filename][locale], null, 2);
-      fileString += "\n"; // Newline at end of file to match Coding Style
-      fs.writeFile(path, fileString, 'utf8', (err)=>

[Thomas Greiner, 2018/01/22 19:49:52]

Coding style: "arrow-spacing" ESLint rule violation

[Thomas Greiner, 2018/01/22 19:49:54]

Coding style: "Double-quoted strings (e.g. "foo") are preferred to single-quoted
strings (e.g. 'foo'), in JavaScript, except to avoid escaping embedded double
quotes, or when assigning inline event handlers."

[saroyanm, 2018/02/28 20:48:26]

Done.

[saroyanm, 2018/02/28 20:48:30]

Done.

-        {
-          if (!err)

[Thomas Greiner, 2018/01/22 19:49:54]

Detail: It's usually simpler to avoid negation where possible. In this case it
doesn't seem to add any value so I'd recommend swapping the blocks.

[saroyanm, 2018/02/28 20:48:31]

Done.

-          {
-            console.log(`Updated: ${path}`);
+            console.error(err);
           }
           else
           {
-            console.log(err);

[Thomas Greiner, 2018/01/22 19:49:53]

You're outputting an error here so why not use `console.error()`?

[saroyanm, 2018/02/28 20:48:27]

Done.

+            console.log(`Updated: ${filePath}`);
           }
         });
     }
   }
 }
 
 /**
- * Parse CSV string and return array
- * @param  {String} csvText Array to convert from

[Thomas Greiner, 2018/01/22 19:49:49]

Detail: We're writing native types in lower-case in adblockpluschrome so I'd
recommend doing the same in our code.

Also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:30]

Done.

- * @return {Array} two dimentional array

[Thomas Greiner, 2018/01/22 19:49:50]

Detail: We're defining arrays using the alternative syntax (e.g. `number[]` or
in this case `string[][]`) in adblockpluschrome so I'd recommend doing the same
in our code.

Also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:30]

Done.

- */
-function csvToArray(csvText)
-{
-  let previouseChar = "";

[Thomas Greiner, 2018/01/22 19:49:50]

Typo: Replace "previouseChar" with "previousChar".

[saroyanm, 2018/02/28 20:48:25]

Done.

-  let row = []; // Holds parsed CSV data representing a row/line 
-  let column = 0; // Pointer of the column in the row
-  let csvArray = []; // Two dimentional array that holds rows
-  let parseSpecialChars = true; // Like comma(,) and quotation(")

[Thomas Greiner, 2018/01/22 19:49:49]

I doubt that we want to get involved into sanitizing the text. Instead we could
leave the CSV parsing up to a third-party module.

Same goes for `arrayToCsv()`.

[saroyanm, 2018/02/28 20:48:23]

Done.

-  for (let charIndex in csvText)
-  {
-    currentChar = csvText[charIndex];
-    if (!row[column])
-      row[column] = "";
-
-    if ('"' == currentChar)
-    {
-      // Double quote is like escaping quote char in CSV
-      if (currentChar === previouseChar && parseSpecialChars)
-        row[column] += currentChar;
-
-      parseSpecialChars = !parseSpecialChars;
-    }
-    else if (currentChar == "," && parseSpecialChars)
-    {
-      currentChar = "";
-      column++; // Update columns, because comma(,) separates columns
-    }
-    else if (currentChar == "\n" && parseSpecialChars)
-    {
-      if ("\r" === previouseChar) // In case of \r\n
-        row[column] = row[column].slice(0, -1);
- 
-      csvArray.push(row);
-      // Reset pointers for the new row
-      row = [];
-      column = 0;
-      currentChar = "";
-    }
-    else
-    {
-      row[column] += currentChar;
-    }
-    previouseChar = currentChar;
-  }
-  csvArray.push(row);
-  return csvArray;
-}
-
-
-/**
- * Convert two dimentional array to the CSV file
- * @param  {Array} csvArray Array to convert from
+ * Convert two dimensional array to the CSV file
+ * @param  {string[][]} csvArray - array to convert from
  */
 function arrayToCsv(csvArray)
 {
-  let dataToWrite = "";
-  for (let row of csvArray)
-  {
-    let columnString = row.reduce((accum, col) =>
-    {
-      // Escape single quote with quote before
-      accum += `","${col.replace(/\"/g, '""')}`;
-      return accum;
-    });
-    dataToWrite += `"${columnString}"\r\n`;
-  }
-  dataToWrite += "\r\n";
-  fs.writeFile(outputFileName, dataToWrite, "utf8", function (err)
-  {
-    if (!err)
-      console.log(`${outputFileName} is created`);
+  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} fileName ex.: "desktop-options.json" 
- * @return {Promise} Promise object

[Thomas Greiner, 2018/01/22 19:49:51]

Detail: This is very non-descriptive because it could mean anything. Instead,
let's describe what the promise represents.

e.g.
@return {Promise<Object>} contents of given file

Also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:32]

Done.

- */
-function readJson(locale, file)

[Thomas Greiner, 2018/01/22 19:49:51]

Detail: "file" is not what you call it in the JSDoc block above.

See also
https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#new-l...

[saroyanm, 2018/02/28 20:48:26]

Done.

-{
-  let path = `${localesDir}/${locale}/${file}`;

[Thomas Greiner, 2018/01/22 19:49:49]

Detail: This variable is only being used once because the `console.log()` below
is commented out so what about inlining the string now?

[saroyanm, 2018/02/28 20:48:25]

We are already using path module here, so I think we can keep assignment to
variable for the readability, I just changed the scope.

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

Acknowledged.

+ * @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) =>
   {
-    fs.readFile(path, (err, data) => { 

[Thomas Greiner, 2018/01/22 19:49:52]

Coding style: "Opening braces always go on their own line."

Also applies to other occurrences.

[saroyanm, 2018/02/28 20:48:27]

Done.

+    let filePath = path.join(localesDir, locale, fileName);
+    fs.readFile(filePath, (err, data) =>
+    {
       if (err)
       {
         reject(err);
       }
       else
       {
-        let json = {};
-        json.filename = file;
-        json.locale = locale;
-        json.strings = JSON.parse(data);
-        resolve(json);

[Thomas Greiner, 2018/01/22 19:49:50]

Detail: Let's simplify this a bit and avoid this temporary variable:

resolve({
  filename, locale,
  strings: JSON.parse(data)
});

[saroyanm, 2018/02/28 20:48:32]

Done.

-      }
-    });
-  }).catch(reason => // Continue Promise.All even if rejected.

[Thomas Greiner, 2018/01/22 19:49:52]

This function should not be aware of where it's called from so that we can
easily reuse it in the future.

[saroyanm, 2018/02/28 20:48:24]

Not sure if it's yet necessary, while we are not outputting anything.

-  { 
-    // Commented out log not to spam the output.
-    // TODO: Think about more meaningful output without spaming
-    // console.log(`Reading ${path} was rejected: ${reason}`);

[Thomas Greiner, 2018/01/22 19:49:49]

Coding style: "Don't leave debug printfs or dumps lying around."

We don't include TODO comments and commented out code in production code which
is also why we use the "no-console" and "no-warning-comment" rules in
eslint-config-eyeo.

[saroyanm, 2018/02/28 20:48:28]

Done.

-  });
-}
-
-/**
- * Reads CSV file
- * @param  {String} file path
- * @return {Promise} Promise object
- */
-function readCsv(filePath)

[Thomas Greiner, 2018/01/22 19:49:51]

This name is a bit misleading since it reads any file, not just CSV files.

[saroyanm, 2018/02/28 20:48:28]

Done.

+        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) => { 
+    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 patch of the folder

[Thomas Greiner, 2018/01/22 19:49:53]

Typo: Replace "patch" with "path"

[saroyanm, 2018/02/28 20:48:25]

Done.

- * @return {Promise} Promise object
+ * @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) => { 
+    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} Promise object containing output from the command
- */
-function executeMercurial(command)
-{
-  // Limit only to Mercurial commands to minimize the missuse risk
-  if (command.substring(0, 3) !== "hg ")
-  {
-    console.error("You are only allowed to run Mercurial commands('hg ...')");

[Thomas Greiner, 2018/01/22 19:49:53]

Why do you allow to pass arbitrary commands if you only allow `hg` in the first
place? Why not instead just pass its CLI arguments?

Regarding the topic of misuse that you mentioned above: I don't know how much of
an issue that really is unless you don't trust the script itself.

[saroyanm, 2018/02/28 20:48:30]

Agree, done.

-    return;
-  }
-
+ * @param  {string} command - mercurial command ex.:"hg ..."
+ * @return {Promise<Object>} output of the command
+ */
+function executeMercurial(commands)
+{
   return new Promise((resolve, reject) =>
   {
-    exec(command, (err, output) =>
+    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: This script exports locales into .csv format

[Thomas Greiner, 2018/01/22 19:49:48]

It can also export locales into JSON format so why not describe it as "Converts
locale files between CSV and JSON formats"?

[saroyanm, 2018/02/28 20:48:27]

Done.

-Usage: node csv-export.js [option] [argument]

[Thomas Greiner, 2018/01/22 19:49:51]

Detail: AFAIK it's not common to prefix scripts with the program you expect them
to be run with - in this case "node".

[saroyanm, 2018/02/28 20:48:24]

Done.

+About: Converts locale files between CSV and JSON formats
+Usage: csv-export.js [option] [argument]
 Options:
-  -f                    Name of the files to be exported ex.: -f firstRun.json

[Thomas Greiner, 2018/01/22 19:49:49]

Only writing "-f" makes it look like it's a binary option so what you'd want to
write instead is "-f [FILENAME]" to indicate that it expects a file name to be
provided. Same applies to the other options.

Therefore the example would also no longer be necessary.

[saroyanm, 2018/02/28 20:48:27]

Done.

-                        option can be used multiple timeString.

[Thomas Greiner, 2018/01/22 19:49:49]

Typo: Replace "timeString" with "times".

[saroyanm, 2018/02/28 20:48:31]

Done.

-                        If ommited all files are being exported
-
-  -o                    Output filename ex.:
+  -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

[Thomas Greiner, 2018/01/22 19:49:50]

I wouldn't know what "default repository" refers to. Based on the code it sounds
like it's the current directory's name so what about clarifying that?

[saroyanm, 2018/02/28 20:48:27]

It's the name of the repository set as default in the paths.
"hg paths default".

Not sure how to improve the description here.

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

Then why not add that to the description? For example: "Name of Mercurial
repository path as shown in 'hg paths'"

-                        If ommited the output fileName is set to
+                        If omitted the output fileName is set to
                         translations-{repo}-{hash}.csv
 
-  -i                    Import file path ex: -i issue-reporter.csv

[Thomas Greiner, 2018/01/22 19:49:51]

From what I see, there are two parameters for specifying the input files: "-f"
for converting JSON to CSV and "-i" for converting CSV to JSON. It's also not
obvious whether you can use "-o" for both conversions or only when exporting to
CSV.

Therefore I'd strongly recommend making this more consistent or splitting it up
into two separate scripts.

[saroyanm, 2018/02/28 20:48:29]

It's suppose to be used with "-f" or without, but not with  "-i". As it's used
for specifying the name of the exported file, but as we don't export anything
using "-i", we don't need it there.

I think best way to communicate that would be to split the file.
But I'd rather address that in the separate issue if it's fine with you.

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

Ok, let's tackle this separately.

+  -i [FILENAME]         Import file path ex: -i issue-reporter.csv
 `;
 
 let arguments = process.argv.slice(2);
 let stopExportScript = false;
-let filesFilter = []; // Filter to be used export to the fileNames inside
+// 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":
-      if (!arguments[i + 1]) // check if argument following option is specified
-      {
-        console.error("Please specify the input filename");
-        stopExportScript = true;

[Thomas Greiner, 2018/01/22 19:49:52]

In Node you can stop the process via `process.exit()`. In case of an error you'd
also pass it an error code (e.g. `process.exit(1)`).

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

[saroyanm, 2018/02/28 20:48:23]

Done, I think the code will become clearer if I separate import and export.

+      // 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])
       {
-        console.error("Please specify the output filename");
-        stopExportScript = true;
+        process.exit("Please specify the output filename");
       }
       else
       {
         outputFileName = arguments[i + 1];
       }
       break;
     case "-i":
       if (!arguments[i + 1])
       {
-        console.error("Please specify the input filename");
+        process.exit("Please specify the import file");
       }
       else
       {
         let importFile = arguments[i + 1];
         importTranslations(importFile);
-      }
-      stopExportScript = true;
+        stopExportScript = true;
+      }
       break;
   }
 }
 
 if (!stopExportScript)
   exportTranslations(filesFilter);