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,477 @@
+/*

[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/>,
+ * 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 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.

+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
+ */
+function exportTranslations(filesFilter)
+{
+  let mercurialCommands = [];
+  mercurialCommands.push(executeMercurial("hg id -i")); // Get Hash
+  mercurialCommands.push(executeMercurial("hg paths default")); // Get repo path
+  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.

+    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);
+  }).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;
+    // Filter files
+    if (filesFilter.length)
+      filesNames = filesNames.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 locale of locales)
+        readJsonPromises.push(readJson(locale, file));
+
+    // 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
+  });
+}
+
+/**
+ * Import strings from the CSV file
+ * @param  {[type]} 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.

+    let dataTreeObj = {};
+    let currentFilename = "";
+    for(let rowId in dataMatrix)
+    {
+      let row = dataMatrix[rowId];
+      let [stringId, description, placeholder] = 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.

+      {
+        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();
+        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] = {};
+
+        // We keep string descriptions only in default locale files
+        if (locale == defaultLocale)
+          localeObj[stringId].description = description;
+
+        localeObj[stringId].message = message;
+        if (placeholder)
+        {
+          let placeholders = placeholder.split("\n");
+          let placeholderName = "";
+          localeObj[stringId].placeholders = placeholders.reduce((acc, item) =>
+          {
+            /*
+             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}`);
+          }
+          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.

+          }
+        });
+    }
+  }
+}
+
+/**
+ * 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
+ */
+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`);
+  });
+}
+
+/**
+ * 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.

+  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.

+      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.

+{
+  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 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
+ */
+function readDir(dir)
+{
+  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} 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;
+  }
+
+  return new Promise((resolve, reject) =>
+  {
+    exec(command, (err, output) =>
+    {
+      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.

+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 firstRun.json -o {hash}-firstRun.csv
+                        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
+                        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.

+`;
+
+let arguments = process.argv.slice(2);
+let stopExportScript = false;
+let filesFilter = []; // Filter to be used export to the fileNames inside
+
+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.

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