Issue 29636585: Issue 6171 - create CSV exporter and importer for translations (Closed)
|
Created:
Dec. 12, 2017, 11:19 a.m. by saroyanm Modified:
Sept. 7, 2018, 2:54 p.m. Visibility:
Public. |
DescriptionUse CLI for information about how to use the script:
node build/csv-export.js -h
Patch Set 1 : #
Total comments: 1
Patch Set 2 : Added the copyright header #
Total comments: 115
Patch Set 3 : Addressed Thomas's comments #
Total comments: 47
Patch Set 4 : Rebased #Patch Set 5 : Addressed most of the comments #
Total comments: 56
Patch Set 6 : Moved the script into build directory and updated the Readme #
Total comments: 12
Patch Set 7 : Addressed Thomas comments #Patch Set 8 : Removed mercurial commands #
Total comments: 16
Patch Set 9 : Addressed Thomas comments #Patch Set 10 : fixed eslint errors #Patch Set 11 : Use consistent naming and promisify fs.writefile #Patch Set 12 : used disable-eslint comment #Patch Set 13 : Do not allow repetitive filenames and Don't use promisify for our functions #
Total comments: 6
Patch Set 14 : Addressed Comments #
Total comments: 2
Patch Set 15 : #
MessagesTotal messages: 34
@Thomas this is ready for the review, let's have a look after the Christmas holidays. https://codereview.adblockplus.org/29636585/diff/29644610/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29644610/csv-export.js#newco... csv-export.js:109: function importTranslations(filePath) Important translations order are different from the crowdin synchronization script. Currently the order of the Strings in locales matching the one in the defaultLocale, while it's not true for existing locales. I think this needs to be fixed as well. Let's discuss that after holidays @Thomas.
This requires quite a significant amount of changes. One of the reasons for that is that it touches on a couple of new topics we haven't discussesd so far in the UI team (e.g. directory structure; JSDoc usage; third-party modules usage). Splitting the code up into smaller chunks (i.e. functions, modules, scripts) should help make it more maintainable though. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1 csv-export.js:1: /* 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:24: let filesNames = []; // ex.: desktop-options.json 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:25: let locales = []; // List of all available locale codes Detail: We tend to put comments in their own line to make them easier to spot if you don't have syntax highlighting enabled. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:27: let outputFileName = "translations-{repo}-{hash}.csv"; Detail: This custom template seems redundant since we can use template literals when constructing the file name. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:31: * @param {[type]} filesFilter Optional parameter which allow include only 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). https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:31: * @param {[type]} filesFilter Optional parameter which allow include only Detail: "[type]" is not a valid type. Also applies to other occurrences. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:31: * @param {[type]} filesFilter Optional parameter which allow include only 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 https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:32: * fileNames in the array, if ommited all files Typo: Replace "ommited" with "omitted" Also applies to other occurrences in this file. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:40: Promise.all(mercurialCommands).then((outputs) => 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:42: // Remove line endings and "+" sign from the end of the hash 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 https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:43: let [hash, path] = outputs.map((item) => item.replace(/\+\n|\n$/, "")); Detail: Seems like you'd want to use `item.trim()` instead to simplify this code. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:44: // Update name of the file to be outputted Typo: Replace "outputted" with "output" https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:46: outputFileName = outputFileName.replace("{repo}", path.split("/").pop()); 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). https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:49: let readDirectories = []; Suggestion: You can avoid this temporary variable if you want by writing this as: return Promise.all([ readDir(…), readDir(…) ]); https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:50: readDirectories.push(readDir(`${localesDir}/${defaultLocale}`)); 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:53: }).then((files) => Detail: This variable is redundant because you could directly write this as `.then(([fileNames, locales]) =>` https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:61: for(let file of filesNames) 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). https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:71: let dataTreeObj = fileObjects.reduce((acc, fileObject) => Detail: Please use descriptive variable names because while I assume that "acc" is short for "accumulated" it is not clear. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:77: let locale = fileObject.locale; Detail: This becomes a bit simpler when using destructuring: let {filename, locale} = fileObject; https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:86: // Create two dimentional strings array that reflects CSV structure Typo: Replace "dimentional" with "dimensional" Also applies to other occurrences. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:87: let localesWithoutDefault = locales.filter((item) => item != defaultLocale); 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:89: for (let file of filesNames) 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? https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:98: // Use yaml-like format for easy extraction, rather sensitive char hacks 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:112: let isTranslated = localeFileObj && localeFileObj[stringID]; 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:130: let dataMatrix = csvToArray(fileObjects); 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:131: let headers = dataMatrix.splice(0, 1)[0]; Detail: This is equivalent to `dataMatrix.shift()` so let's keep it simple. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:142: if (stringId.endsWith(".json")) // Check if it's the filename row 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:151: for (let i = 3; i < headers.length; i++) 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(); … https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:206: * Write locale files according to dataTreeObj which look like: Detail: There's a redundant "which look like:" in this sentence. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:219: fs.writeFile(path, fileString, 'utf8', (err)=> 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." https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:219: fs.writeFile(path, fileString, 'utf8', (err)=> Coding style: "arrow-spacing" ESLint rule violation https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:221: if (!err) 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:227: console.log(err); You're outputting an error here so why not use `console.error()`? https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:236: * @param {String} csvText Array to convert from 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:237: * @return {Array} two dimentional array 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:241: let previouseChar = ""; Typo: Replace "previouseChar" with "previousChar". https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:245: let parseSpecialChars = true; // Like comma(,) and quotation(") 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()`. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:316: * @return {Promise} Promise object 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:318: function readJson(locale, file) 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... https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:320: let path = `${localesDir}/${locale}/${file}`; Detail: This variable is only being used once because the `console.log()` below is commented out so what about inlining the string now? https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:323: fs.readFile(path, (err, data) => { Coding style: "Opening braces always go on their own line." Also applies to other occurrences. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:334: resolve(json); Detail: Let's simplify this a bit and avoid this temporary variable: resolve({ filename, locale, strings: JSON.parse(data) }); https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:337: }).catch(reason => // Continue Promise.All even if rejected. This function should not be aware of where it's called from so that we can easily reuse it in the future. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:341: // console.log(`Reading ${path} was rejected: ${reason}`); 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:350: function readCsv(filePath) This name is a bit misleading since it reads any file, not just CSV files. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:365: * @param {String} dir patch of the folder Typo: Replace "patch" with "path" https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:391: console.error("You are only allowed to run Mercurial commands('hg ...')"); 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:409: About: This script exports locales into .csv format It can also export locales into JSON format so why not describe it as "Converts locale files between CSV and JSON formats"? https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:410: Usage: node csv-export.js [option] [argument] Detail: AFAIK it's not common to prefix scripts with the program you expect them to be run with - in this case "node". https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:412: -f Name of the files to be exported ex.: -f firstRun.json 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:413: option can be used multiple timeString. Typo: Replace "timeString" with "times". https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:420: {repo} - Name of the "Default" repository 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? https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:424: -i Import file path ex: -i issue-reporter.csv 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:443: stopExportScript = true; 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
https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:24: let filesNames = []; // ex.: desktop-options.json On 2018/01/22 19:49:52, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:25: let locales = []; // List of all available locale codes On 2018/01/22 19:49:53, Thomas Greiner wrote: > Detail: We tend to put comments in their own line to make them easier to spot if > you don't have syntax highlighting enabled. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:27: let outputFileName = "translations-{repo}-{hash}.csv"; On 2018/01/22 19:49:53, Thomas Greiner wrote: > Detail: This custom template seems redundant since we can use template literals > when constructing the file name. We can't pass Template literals using CLI, until I missunderstood your question. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:31: * @param {[type]} filesFilter Optional parameter which allow include only On 2018/01/22 19:49:49, Thomas Greiner wrote: > 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 Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:31: * @param {[type]} filesFilter Optional parameter which allow include only On 2018/01/22 19:49:51, Thomas Greiner wrote: > Detail: "[type]" is not a valid type. > > Also applies to other occurrences. Right, it's the default value generated by DocBlockr. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:31: * @param {[type]} filesFilter Optional parameter which allow include only On 2018/01/22 19:49:51, Thomas Greiner wrote: > Detail: "[type]" is not a valid type. > > Also applies to other occurrences. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:32: * fileNames in the array, if ommited all files On 2018/01/22 19:49:51, Thomas Greiner wrote: > Typo: Replace "ommited" with "omitted" > > Also applies to other occurrences in this file. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:40: Promise.all(mercurialCommands).then((outputs) => On 2018/01/22 19:49:50, Thomas Greiner wrote: > 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. I have simplified this function, can try to simplify even more with next patch. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:42: // Remove line endings and "+" sign from the end of the hash On 2018/01/22 19:49:49, Thomas Greiner wrote: > 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 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 ? https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:43: let [hash, path] = outputs.map((item) => item.replace(/\+\n|\n$/, "")); On 2018/01/22 19:49:52, Thomas Greiner wrote: > Detail: Seems like you'd want to use `item.trim()` instead to simplify this > code. If not the "+" sign the trim would have worked here probably. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:44: // Update name of the file to be outputted On 2018/01/22 19:49:54, Thomas Greiner wrote: > Typo: Replace "outputted" with "output" Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:46: outputFileName = outputFileName.replace("{repo}", path.split("/").pop()); On 2018/01/22 19:49:48, Thomas Greiner wrote: > 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). Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:49: let readDirectories = []; On 2018/01/22 19:49:51, Thomas Greiner wrote: > Suggestion: You can avoid this temporary variable if you want by writing this > as: > > return Promise.all([ > readDir(…), > readDir(…) > ]); Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:50: readDirectories.push(readDir(`${localesDir}/${defaultLocale}`)); On 2018/01/22 19:49:52, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:53: }).then((files) => On 2018/01/22 19:49:48, Thomas Greiner wrote: > Detail: This variable is redundant because you could directly write this as > `.then(([fileNames, locales]) =>` I think I can't as I assigning the variables to the ones declared in the Global scope. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:61: for(let file of filesNames) On 2018/01/22 19:49:50, Thomas Greiner wrote: > 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). Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:71: let dataTreeObj = fileObjects.reduce((acc, fileObject) => On 2018/01/22 19:49:54, Thomas Greiner wrote: > Detail: Please use descriptive variable names because while I assume that "acc" > is short for "accumulated" it is not clear. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:77: let locale = fileObject.locale; On 2018/01/22 19:49:50, Thomas Greiner wrote: > Detail: This becomes a bit simpler when using destructuring: > > let {filename, locale} = fileObject; Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:86: // Create two dimentional strings array that reflects CSV structure On 2018/01/22 19:49:49, Thomas Greiner wrote: > Typo: Replace "dimentional" with "dimensional" > > Also applies to other occurrences. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:87: let localesWithoutDefault = locales.filter((item) => item != defaultLocale); On 2018/01/22 19:49:54, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:89: for (let file of filesNames) On 2018/01/22 19:49:53, Thomas Greiner wrote: > 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? Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:98: // Use yaml-like format for easy extraction, rather sensitive char hacks On 2018/01/22 19:49:54, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:112: let isTranslated = localeFileObj && localeFileObj[stringID]; On 2018/01/22 19:49:53, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:130: let dataMatrix = csvToArray(fileObjects); On 2018/01/22 19:49:51, Thomas Greiner wrote: > 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. Done, with finding csvToArray module. Haven't thought yet about optimizing this implementation, let's keep it for next patch. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:131: let headers = dataMatrix.splice(0, 1)[0]; On 2018/01/22 19:49:51, Thomas Greiner wrote: > Detail: This is equivalent to `dataMatrix.shift()` so let's keep it simple. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:142: if (stringId.endsWith(".json")) // Check if it's the filename row On 2018/01/22 19:49:52, Thomas Greiner wrote: > 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. I agree, but you are right let's first address all existing issues comments/issues and give a thought about this again. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:151: for (let i = 3; i < headers.length; i++) On 2018/01/22 19:49:50, Thomas Greiner wrote: > 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 Done. > 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(); > … https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:206: * Write locale files according to dataTreeObj which look like: On 2018/01/22 19:49:49, Thomas Greiner wrote: > Detail: There's a redundant "which look like:" in this sentence. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:219: fs.writeFile(path, fileString, 'utf8', (err)=> On 2018/01/22 19:49:52, Thomas Greiner wrote: > Coding style: "arrow-spacing" ESLint rule violation Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:219: fs.writeFile(path, fileString, 'utf8', (err)=> On 2018/01/22 19:49:54, Thomas Greiner wrote: > 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." Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:221: if (!err) On 2018/01/22 19:49:54, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:227: console.log(err); On 2018/01/22 19:49:53, Thomas Greiner wrote: > You're outputting an error here so why not use `console.error()`? Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:236: * @param {String} csvText Array to convert from On 2018/01/22 19:49:49, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:237: * @return {Array} two dimentional array On 2018/01/22 19:49:50, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:241: let previouseChar = ""; On 2018/01/22 19:49:50, Thomas Greiner wrote: > Typo: Replace "previouseChar" with "previousChar". Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:245: let parseSpecialChars = true; // Like comma(,) and quotation(") On 2018/01/22 19:49:49, Thomas Greiner wrote: > 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()`. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:316: * @return {Promise} Promise object On 2018/01/22 19:49:51, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:318: function readJson(locale, file) On 2018/01/22 19:49:51, Thomas Greiner wrote: > 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... Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:320: let path = `${localesDir}/${locale}/${file}`; On 2018/01/22 19:49:49, Thomas Greiner wrote: > Detail: This variable is only being used once because the `console.log()` below > is commented out so what about inlining the string now? We are already using path module here, so I think we can keep assignment to variable for the readability, I just changed the scope. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:323: fs.readFile(path, (err, data) => { On 2018/01/22 19:49:52, Thomas Greiner wrote: > Coding style: "Opening braces always go on their own line." > > Also applies to other occurrences. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:334: resolve(json); On 2018/01/22 19:49:50, Thomas Greiner wrote: > Detail: Let's simplify this a bit and avoid this temporary variable: > > resolve({ > filename, locale, > strings: JSON.parse(data) > }); Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:337: }).catch(reason => // Continue Promise.All even if rejected. On 2018/01/22 19:49:52, Thomas Greiner wrote: > This function should not be aware of where it's called from so that we can > easily reuse it in the future. Not sure if it's yet necessary, while we are not outputting anything. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:341: // console.log(`Reading ${path} was rejected: ${reason}`); On 2018/01/22 19:49:49, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:350: function readCsv(filePath) On 2018/01/22 19:49:51, Thomas Greiner wrote: > This name is a bit misleading since it reads any file, not just CSV files. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:365: * @param {String} dir patch of the folder On 2018/01/22 19:49:53, Thomas Greiner wrote: > Typo: Replace "patch" with "path" Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:391: console.error("You are only allowed to run Mercurial commands('hg ...')"); On 2018/01/22 19:49:53, Thomas Greiner wrote: > 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. Agree, done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:409: About: This script exports locales into .csv format On 2018/01/22 19:49:48, Thomas Greiner wrote: > It can also export locales into JSON format so why not describe it as "Converts > locale files between CSV and JSON formats"? Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:410: Usage: node csv-export.js [option] [argument] On 2018/01/22 19:49:51, Thomas Greiner wrote: > Detail: AFAIK it's not common to prefix scripts with the program you expect them > to be run with - in this case "node". Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:412: -f Name of the files to be exported ex.: -f firstRun.json On 2018/01/22 19:49:49, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:413: option can be used multiple timeString. On 2018/01/22 19:49:49, Thomas Greiner wrote: > Typo: Replace "timeString" with "times". Done. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:420: {repo} - Name of the "Default" repository On 2018/01/22 19:49:50, Thomas Greiner wrote: > 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? It's the name of the repository set as default in the paths. "hg paths default". Not sure how to improve the description here. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:424: -i Import file path ex: -i issue-reporter.csv On 2018/01/22 19:49:51, Thomas Greiner wrote: > 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. 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:443: stopExportScript = true; On 2018/01/22 19:49:52, Thomas Greiner wrote: > 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 Done, I think the code will become clearer if I separate import and export.
Thanks Thomas for detailed review, this is already good enough I think for the second round of reviews. There are couple of comments that still need to be addressed, but I think would be great if you can give a go for another review when you have time. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1 csv-export.js:1: /* On 2018/01/22 19:49:52, Thomas Greiner wrote: > 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. I'll address this in a separate patch. Moving this to the buildtools directory causes an issue after running "ensure_dependencies.py". https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, Moving this file to the buildtools directory still needs to be addressed -> https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1 https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:200: for (let locale in dataTreeObj[fileName]) 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.
https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:27: let outputFileName = "translations-{repo}-{hash}.csv"; On 2018/02/28 20:48:22, saroyanm wrote: > We can't pass Template literals using CLI, until I missunderstood your question. 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:42: // Remove line endings and "+" sign from the end of the hash On 2018/02/28 20:48:32, saroyanm wrote: > 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 ? 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. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:53: }).then((files) => On 2018/02/28 20:48:32, saroyanm wrote: > I think I can't as I assigning the variables to the ones declared in the Global > scope. 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`. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:320: let path = `${localesDir}/${locale}/${file}`; On 2018/02/28 20:48:25, saroyanm wrote: > On 2018/01/22 19:49:49, Thomas Greiner wrote: > > Detail: This variable is only being used once because the `console.log()` > below > > is commented out so what about inlining the string now? > > We are already using path module here, so I think we can keep assignment to > variable for the readability, I just changed the scope. Acknowledged. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:420: {repo} - Name of the "Default" repository On 2018/02/28 20:48:27, saroyanm wrote: > It's the name of the repository set as default in the paths. > "hg paths default". > > Not sure how to improve the description here. Then why not add that to the description? For example: "Name of Mercurial repository path as shown in 'hg paths'" https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:424: -i Import file path ex: -i issue-reporter.csv On 2018/02/28 20:48:29, saroyanm wrote: > 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. Ok, let's tackle this separately. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2018/02/28 20:57:34, saroyanm wrote: > Moving this file to the buildtools directory still needs to be addressed -> > https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1 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. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:175: localeObj[stringId] = {}; 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]`. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:200: for (let locale in dataTreeObj[fileName]) On 2018/02/28 20:57:34, saroyanm wrote: > 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. 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? https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:231: fs.writeFile(outputFileName, output, "utf8", function (err) 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. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:234: console.log(`${outputFileName} is created`); 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. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:242: * @param {string} file - ex.: "desktop-options.json" 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... https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:243: * @return {Promise<Object>} fileName, locale and Strings of locale file Typo: Replace "Strings" with "strings" https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:261: // Continue Promise.All even if rejected. Detail: Why? Not being able to read from a JSON file sounds like a legitimate reason to stop the export. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:287: * @return {Promise<Object>} array of folders Suggestion: Technically, those can be either folders or regular files so calling them files may be more appropriate. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:287: * @return {Promise<Object>} array of folders Detail: The return type is `Promise<string[]>`. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:289: function readDir(dir) 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 https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:312: exec(`hg ${commands.join(" ")}`, (err, output) => 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... https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:332: -f firstRun.json -o {hash}-firstRun.csv 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. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:359: process.exit("Please specify the input filename"); This is not how you call `process.exit()`. See https://nodejs.org/api/process.html#process_process_exit_code.
Finished reviewing the remaining parts of the code I didn't get around to before earlier than anticipated so here you go. https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newco... csv-export.js:40: Promise.all(mercurialCommands).then((outputs) => On 2018/02/28 20:48:23, saroyanm wrote: > I have simplified this function, can try to simplify even more with next patch. That'd be great, thanks. The more we can disconnect functions from each other the easier it should be to maintain the code. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, Mind mentioning this script in the README? Preferrably, including a small example of the CSV file format it imports/exports. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:138: return csvParser(fileObjects, {relax_column_count: true}); Why do we end up with an inconsistent number of columns? Aren't we filling-in each cell?
https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2018/03/19 18:28:04, Thomas Greiner wrote: > On 2018/02/28 20:57:34, saroyanm wrote: > > Moving this file to the buildtools directory still needs to be addressed -> > > > https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1 > > 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. Acknowledged, I'll move this to the "build" directory. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2018/03/19 18:54:13, Thomas Greiner wrote: > Mind mentioning this script in the README? Preferrably, including a small > example of the CSV file format it imports/exports. Good point. I'll add information in the README as well. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:138: return csvParser(fileObjects, {relax_column_count: true}); On 2018/03/19 18:54:13, Thomas Greiner wrote: > Why do we end up with an inconsistent number of columns? Aren't we filling-in > each cell? Meeting note: We will use new column called filename and add missing commas when the file is generated. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:175: localeObj[stringId] = {}; On 2018/03/19 18:28:02, Thomas Greiner wrote: > 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]`. Acknowledged. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:200: for (let locale in dataTreeObj[fileName]) On 2018/03/19 18:28:04, Thomas Greiner wrote: > On 2018/02/28 20:57:34, saroyanm wrote: > > 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. > > 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? 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. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:231: fs.writeFile(outputFileName, output, "utf8", function (err) On 2018/03/19 18:28:03, Thomas Greiner wrote: > 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. Acknowledged. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:234: console.log(`${outputFileName} is created`); On 2018/03/19 18:28:04, Thomas Greiner wrote: > 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. Acknowledged. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:289: function readDir(dir) On 2018/03/19 18:28:04, Thomas Greiner wrote: > 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 Agree.
This is ready for review. I did created two followup issues: * https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/75 * https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/74 https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2018/04/26 17:53:52, saroyanm wrote: > On 2018/03/19 18:28:04, Thomas Greiner wrote: > > On 2018/02/28 20:57:34, saroyanm wrote: > > > Moving this file to the buildtools directory still needs to be addressed -> > > > > > > https://codereview.adblockplus.org/29636585/diff/29645625/csv-export.js#newcode1 > > > > 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. > > Acknowledged, I'll move this to the "build" directory. Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newcode2 csv-export.js:2: * This file is part of Adblock Plus <https://adblockplus.org/>, On 2018/04/26 17:53:52, saroyanm wrote: > On 2018/03/19 18:54:13, Thomas Greiner wrote: > > Mind mentioning this script in the README? Preferrably, including a small > > example of the CSV file format it imports/exports. > > Good point. I'll add information in the README as well. Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:58: return Promise.all([readDir(path.join(localesDir, defaultLocale)), 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) https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:107: for (let fileName of fileNames) I have this information in fileObjects, I shouldn't use separate array to read filenames and locales. (Done) https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:138: return csvParser(fileObjects, {relax_column_count: true}); On 2018/04/26 17:53:51, saroyanm wrote: > On 2018/03/19 18:54:13, Thomas Greiner wrote: > > Why do we end up with an inconsistent number of columns? Aren't we filling-in > > each cell? > > Meeting note: We will use new column called filename and add missing commas when > the file is generated. Apparently we were generating right amount of commas. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:138: return csvParser(fileObjects, {relax_column_count: true}); On 2018/03/19 18:54:13, Thomas Greiner wrote: > Why do we end up with an inconsistent number of columns? Aren't we filling-in > each cell? Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:175: localeObj[stringId] = {}; On 2018/03/19 18:28:02, Thomas Greiner wrote: > 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]`. Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:200: for (let locale in dataTreeObj[fileName]) On 2018/04/26 17:53:52, saroyanm wrote: > On 2018/03/19 18:28:04, Thomas Greiner wrote: > > On 2018/02/28 20:57:34, saroyanm wrote: > > > 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. > > > > 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? > > 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. Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:231: fs.writeFile(outputFileName, output, "utf8", function (err) On 2018/03/19 18:28:03, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:234: console.log(`${outputFileName} is created`); On 2018/03/19 18:28:04, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:242: * @param {string} file - ex.: "desktop-options.json" On 2018/03/19 18:28:04, Thomas Greiner wrote: > 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... Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:243: * @return {Promise<Object>} fileName, locale and Strings of locale file On 2018/03/19 18:28:03, Thomas Greiner wrote: > Typo: Replace "Strings" with "strings" Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:261: // Continue Promise.All even if rejected. On 2018/03/19 18:28:02, Thomas Greiner wrote: > Detail: Why? Not being able to read from a JSON file sounds like a legitimate > reason to stop the export. 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. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:287: * @return {Promise<Object>} array of folders On 2018/03/19 18:28:03, Thomas Greiner wrote: > Detail: The return type is `Promise<string[]>`. Irrelevant in the new patch. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:289: function readDir(dir) On 2018/03/19 18:28:04, Thomas Greiner wrote: > 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 Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:312: exec(`hg ${commands.join(" ")}`, (err, output) => On 2018/03/19 18:28:04, Thomas Greiner wrote: > 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... Done. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:332: -f firstRun.json -o {hash}-firstRun.csv On 2018/03/19 18:28:02, Thomas Greiner wrote: > 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. Not sure I understand the comment. I'm not passing arguments? https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:359: process.exit("Please specify the input filename"); On 2018/03/19 18:28:04, Thomas Greiner wrote: > This is not how you call `process.exit()`. > > See https://nodejs.org/api/process.html#process_process_exit_code. Done. https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:313: switch (argv[i]) It would be great to use a library like [minimist](https://www.npmjs.com/package/minimist) to handle this, but as this implementation might change [here](https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/74) I think we can tackle that separately?
https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:207: console.log(`Updated: ${filePath}`); console.log doesn't pass eslint -> Unexpected console statement. (no-console) This sounds more like an exceptional case for the csv-exporter that we would like to have those outputs.
Adding myself to reviewers for future reference.
https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:58: return Promise.all([readDir(path.join(localesDir, defaultLocale)), On 2018/05/04 13:51:08, saroyanm wrote: > 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) Acknowledged. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:107: for (let fileName of fileNames) On 2018/05/04 13:51:10, saroyanm wrote: > I have this information in fileObjects, I shouldn't use separate array to read > filenames and locales. > > (Done) Acknowledged. https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... csv-export.js:332: -f firstRun.json -o {hash}-firstRun.csv On 2018/05/04 13:51:10, saroyanm wrote: > Not sure I understand the comment. I'm not passing arguments? 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` https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:18: /* globals require, process */ Detail: No need to list `require` as a global anymore because we added it globally in https://gitlab.com/eyeo/adblockplus/adblockplusui/commit/08a2c66736a1f489fe76... https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:28: const readDir = promisify(fs.readdir); This variable is unused. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:29: const readFile = promisify(fs.readFile); This variable is unused so let's use it instead of `fs.readFile()`. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:31: const readJsonPromised = promisify(readJson); Why do you call `promisify()` on your own function? You can just convert it to return a promise. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:36: let headers = ["filename", "StringID", "Description", "Placeholders", Detail: Unlike the other headers, "filename" doesn't start with a capital character. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:56: item.stdout.replace(/\+\n|\n$/, "")); Suggestion: You're calling the array "outputs" but the items are called "item" so why not call them "output" to make it less generic? https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:62: return glob(`${localesDir}/**/*.json`, {}); Detail: The second argument is optional according to https://github.com/isaacs/node-glob#globpattern-options-cb. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:72: * @param {Array} fileObjects - array of file objects created by readJson Detail: Arrays of objects are described as `Object[]`. Note that `Array<Object>` (or more accurately `Array.<Object>`) would be another option but that'd be inconsistent which how we describe arrays throughout our code. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => This `reduce()` is no longer just populating a single object (i.e. `dataTreeObj`) but also `locales` and `fileNames`. In that regard a regular loop seems more appropriate to avoid confusion. You could then even split this off into its own function if you want. e.g. let dataTree = Object.create(null); let fileNames = []; let locales = []; for (let fileObject of fileObjects) { if (!fileObject) continue; let {fileName, locale, strings} = fileObject; if (locale != defaultLocale && !locales.include(locale)) { locales.push(locale); } if (!filesFilter.length || filesFilter.includes(fileName)) { fileNames.push(fileName); } if (!(fileName in dataTree)) { dataTree[fileName] = Object.create(null); } dataTree[fileName][locale] = strings; } locales.sort(); https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:100: locales = locales.filter((locale) => locale != defaultLocale).sort(); Why do you sort the locales? That step didn't exist in the previous revision so I'm wondering whether it is necessary. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:193: let orderedJSON = orderJSON(dataTreeObj[fileName][locale]); Suggestion: Usually, we'd use the word "sort" instead of "order". A good reason I could think of is because "to order" can also mean "to make an order" or "to command". https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:215: * This function currently rely on nodeJS to sort the object by keys Typo: Replace "rely" with "relies". https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:215: * This function currently rely on nodeJS to sort the object by keys Detail: Technically, it relies on V8's implementation, not Node's. However, I found https://github.com/nodejs/node/issues/15628#issuecomment-332588533 which states that in ES2016 the order is now deterministic based on the order in which properties got added. Therefore we could remove this comment and instead simply describe what this function is for. Alternatively, we could use https://github.com/substack/json-stable-stringify which was suggested in the comment I linked to above. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:270: callback(null, {fileName: base, locale: dir.split("/").pop(), The path delimiter is OS-specific so please use `path.delimiter` instead. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:270: callback(null, {fileName: base, locale: dir.split("/").pop(), Detail: Let's avoid doing multiple things in a single statement to make it easier to maintain. i.e. let locale = dir.split("/").pop(); let strings = JSON.parse(data); callback(null, {fileName: base, locale, strings}), https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:283: process.exit(); We should pass a number that's not `0` when exiting the process with an error. That way other CLI programs who call our script know whether the import/export failed. Passing `1` would be sufficient for now. See https://nodejs.org/api/process.html#process_process_exit_code https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:317: stopExportScript = true; Suggestion: What about moving the argument processing logic into its own function which returns this boolean? That way you could just do `return true` here and avoid the global `stopExportScript` variable. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:320: // check if argument following option is specified Detail: The code seems self-explanatory so maybe best to remove this comment. At least it's not used for the other occurrences. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:325: else Detail: This else-statement is redundant because we're exiting in the if-statement. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:335: else Detail: This else-statement is redundant because we're exiting in the if-statement. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:345: else Detail: This else-statement is redundant because we're exiting in the if-statement. https://codereview.adblockplus.org/29636585/diff/29770564/README.md File README.md (right): https://codereview.adblockplus.org/29636585/diff/29770564/README.md#newcode183 README.md:183: | desktop-options.json | options_whitelist_placeholder | Input placeholder prefix | {"domain":{"content":"$1"}} | e.g. $domain$ | | | | Don't we have translations for this string? Note that we don't need to use real strings for this example so feel free to make up your own mock data for this example to illustrate the format. https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:313: switch (argv[i]) On 2018/05/04 13:51:11, saroyanm wrote: > It would be great to use a library like > [minimist](https://www.npmjs.com/package/minimist) to handle this, but as this > implementation might change > [here](https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/74) I think we > can tackle that separately? Agreed.
https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:31: const readJsonPromised = promisify(readJson); On 2018/05/07 15:16:36, Thomas Greiner wrote: > Why do you call `promisify()` on your own function? You can just convert it to > return a promise. Because of current comment -> https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... I thought you want to use promisify to simplify the Promise implementation. https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:49: mercurialCommands.push(execFile("hg", ["id", "-i"])); This will not work in git. Alternative: mercurialCommands.push(execFile("git", ["rev-parse", "HEAD"])); https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:51: mercurialCommands.push(execFile("hg", ["paths", "default"])); This will not work in git. Alternative: mercurialCommands.push(execFile("git", ["config", "--get", "remote.origin.url"]));
https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:18: /* globals require, process */ On 2018/05/07 15:16:33, Thomas Greiner wrote: > Detail: No need to list `require` as a global anymore because we added it > globally in > https://gitlab.com/eyeo/adblockplus/adblockplusui/commit/08a2c66736a1f489fe76... Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:28: const readDir = promisify(fs.readdir); On 2018/05/07 15:16:34, Thomas Greiner wrote: > This variable is unused. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:29: const readFile = promisify(fs.readFile); On 2018/05/07 15:16:37, Thomas Greiner wrote: > This variable is unused so let's use it instead of `fs.readFile()`. The variable is used. see, line: 131 https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:36: let headers = ["filename", "StringID", "Description", "Placeholders", On 2018/05/07 15:16:38, Thomas Greiner wrote: > Detail: Unlike the other headers, "filename" doesn't start with a capital > character. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:56: item.stdout.replace(/\+\n|\n$/, "")); On 2018/05/07 15:16:34, Thomas Greiner wrote: > Suggestion: You're calling the array "outputs" but the items are called "item" > so why not call them "output" to make it less generic? Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:62: return glob(`${localesDir}/**/*.json`, {}); On 2018/05/07 15:16:37, Thomas Greiner wrote: > Detail: The second argument is optional according to > https://github.com/isaacs/node-glob#globpattern-options-cb. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:72: * @param {Array} fileObjects - array of file objects created by readJson On 2018/05/07 15:16:37, Thomas Greiner wrote: > Detail: Arrays of objects are described as `Object[]`. > > Note that `Array<Object>` (or more accurately `Array.<Object>`) would be another > option but that'd be inconsistent which how we describe arrays throughout our > code. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/05/07 15:16:38, Thomas Greiner wrote: > This `reduce()` is no longer just populating a single object (i.e. > `dataTreeObj`) but also `locales` and `fileNames`. In that regard a regular loop > seems more appropriate to avoid confusion. > > You could then even split this off into its own function if you want. > > e.g. > > let dataTree = Object.create(null); > let fileNames = []; > let locales = []; > > for (let fileObject of fileObjects) > { > if (!fileObject) > continue; > > let {fileName, locale, strings} = fileObject; > > if (locale != defaultLocale && !locales.include(locale)) > { > locales.push(locale); > } > > if (!filesFilter.length || filesFilter.includes(fileName)) > { > fileNames.push(fileName); > } > > if (!(fileName in dataTree)) > { > dataTree[fileName] = Object.create(null); > } > dataTree[fileName][locale] = strings; > } > > locales.sort(); Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:100: locales = locales.filter((locale) => locale != defaultLocale).sort(); On 2018/05/07 15:16:34, Thomas Greiner wrote: > Why do you sort the locales? That step didn't exist in the previous revision so > I'm wondering whether it is necessary. I think I did that to have consistent CSV file generated with ordered locales. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:193: let orderedJSON = orderJSON(dataTreeObj[fileName][locale]); On 2018/05/07 15:16:37, Thomas Greiner wrote: > Suggestion: Usually, we'd use the word "sort" instead of "order". A good reason > I could think of is because "to order" can also mean "to make an order" or "to > command". Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:215: * This function currently rely on nodeJS to sort the object by keys On 2018/05/07 15:16:35, Thomas Greiner wrote: > Detail: Technically, it relies on V8's implementation, not Node's. Done > However, I found > https://github.com/nodejs/node/issues/15628#issuecomment-332588533 which states > that in ES2016 the order is now deterministic based on the order in which > properties got added. > Therefore we could remove this comment and instead simply describe what this > function is for. > Alternatively, we could use https://github.com/substack/json-stable-stringify > which was suggested in the comment I linked to above. While current version works reliable enough, I suggest we discuss better implementation in the separate ticket. Let's go with the "Detail" fix for now. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:215: * This function currently rely on nodeJS to sort the object by keys On 2018/05/07 15:16:36, Thomas Greiner wrote: > Typo: Replace "rely" with "relies". Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:270: callback(null, {fileName: base, locale: dir.split("/").pop(), On 2018/05/07 15:16:35, Thomas Greiner wrote: > Detail: Let's avoid doing multiple things in a single statement to make it > easier to maintain. > > i.e. > let locale = dir.split("/").pop(); > let strings = JSON.parse(data); > callback(null, {fileName: base, locale, strings}), Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:270: callback(null, {fileName: base, locale: dir.split("/").pop(), On 2018/05/07 15:16:34, Thomas Greiner wrote: > The path delimiter is OS-specific so please use `path.delimiter` instead. Done. I think you meant `path.sep` https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:283: process.exit(); On 2018/05/07 15:16:35, Thomas Greiner wrote: > We should pass a number that's not `0` when exiting the process with an error. > That way other CLI programs who call our script know whether the import/export > failed. > > Passing `1` would be sufficient for now. > > See https://nodejs.org/api/process.html#process_process_exit_code OOps, thanks, done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:317: stopExportScript = true; On 2018/05/07 15:16:38, Thomas Greiner wrote: > Suggestion: What about moving the argument processing logic into its own > function which returns this boolean? That way you could just do `return true` > here and avoid the global `stopExportScript` variable. As was mentioned here -> https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... This logic going change anyway, so I think best will be to tackle that separately. Please let me know if you want me to do that changes here. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:320: // check if argument following option is specified On 2018/05/07 15:16:37, Thomas Greiner wrote: > Detail: The code seems self-explanatory so maybe best to remove this comment. At > least it's not used for the other occurrences. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:325: else On 2018/05/07 15:16:34, Thomas Greiner wrote: > Detail: This else-statement is redundant because we're exiting in the > if-statement. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:335: else On 2018/05/07 15:16:35, Thomas Greiner wrote: > Detail: This else-statement is redundant because we're exiting in the > if-statement. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:345: else On 2018/05/07 15:16:35, Thomas Greiner wrote: > Detail: This else-statement is redundant because we're exiting in the > if-statement. Done. https://codereview.adblockplus.org/29636585/diff/29770564/README.md File README.md (right): https://codereview.adblockplus.org/29636585/diff/29770564/README.md#newcode183 README.md:183: | desktop-options.json | options_whitelist_placeholder | Input placeholder prefix | {"domain":{"content":"$1"}} | e.g. $domain$ | | | | On 2018/05/07 15:16:38, Thomas Greiner wrote: > Don't we have translations for this string? We have I think I used translation from the generated CSV. > Note that we don't need to use real strings for this example so feel free to > make up your own mock data for this example to illustrate the format. What is the problem with real strings ?
This is ready for review. CC: @wspee I have removed the logic that were using commit hash to generate file, as it was mercurial specific, we might still want to think about naming and implement it separately if we think this is a feature we need. https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:49: mercurialCommands.push(execFile("hg", ["id", "-i"])); On 2018/05/15 18:59:49, saroyanm wrote: > This will not work in git. > > Alternative: mercurialCommands.push(execFile("git", ["rev-parse", "HEAD"])); As discussed: We better remove this mercurial specific logic and introduce it back if we think we really need it.
https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:29: const readFile = promisify(fs.readFile); On 2018/05/16 17:06:33, saroyanm wrote: > On 2018/05/07 15:16:37, Thomas Greiner wrote: > > This variable is unused so let's use it instead of `fs.readFile()`. > > The variable is used. see, line: 131 Still, you're using `fs.readFile()` in `readJson()`. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:31: const readJsonPromised = promisify(readJson); On 2018/05/15 18:59:48, saroyanm wrote: > Because of current comment -> > https://codereview.adblockplus.org/29636585/diff/29711668/csv-export.js#newco... > > > I thought you want to use promisify to simplify the Promise implementation. That's because we cannot change how native Node functions are implemented so all we can do is convert the callback into a promise using `promisify()`. However, for functions that we create ourselves we don't need to implement them using callbacks but can return the promise directly and thereby don't need this conversion step. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/05/16 17:06:32, saroyanm wrote: > Done. You replaced the `reduce()` call with a for-of loop now. It still contains the same logic as before though, which means that there's still redundant loops for populating the `locales` and `fileNames` arrays. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:100: locales = locales.filter((locale) => locale != defaultLocale).sort(); On 2018/05/16 17:06:33, saroyanm wrote: > On 2018/05/07 15:16:34, Thomas Greiner wrote: > > Why do you sort the locales? That step didn't exist in the previous revision > so > > I'm wondering whether it is necessary. > > I think I did that to have consistent CSV file generated with ordered locales. Acknowledged. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:193: let orderedJSON = orderJSON(dataTreeObj[fileName][locale]); On 2018/05/16 17:06:31, saroyanm wrote: > Done. Note that while you renamed the variable, the function is still called "orderJSON". Also note the inconsistency in the capitalization. i.e. - csvFromJsonFileObjects - orderJSON - orderedJSON - readJson - writeJson It'd be great if we could agree on a naming convention for such cases. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:215: * This function currently rely on nodeJS to sort the object by keys On 2018/05/16 17:06:35, saroyanm wrote: > On 2018/05/07 15:16:35, Thomas Greiner wrote: > > Detail: Technically, it relies on V8's implementation, not Node's. > Done > > However, I found > > https://github.com/nodejs/node/issues/15628#issuecomment-332588533 which > states > > that in ES2016 the order is now deterministic based on the order in which > > properties got added. > > Therefore we could remove this comment and instead simply describe what this > > function is for. > > > Alternatively, we could use https://github.com/substack/json-stable-stringify > > which was suggested in the comment I linked to above. > > While current version works reliable enough, I suggest we discuss better > implementation in the separate ticket. > Let's go with the "Detail" fix for now. Acknowledged. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:270: callback(null, {fileName: base, locale: dir.split("/").pop(), On 2018/05/16 17:06:32, saroyanm wrote: > Done. I think you meant `path.sep` Yep, you're right. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:317: stopExportScript = true; On 2018/05/16 17:06:35, saroyanm wrote: > As was mentioned here -> > https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... > > > This logic going change anyway, so I think best will be to tackle that > separately. > Please let me know if you want me to do that changes here. Ok. https://codereview.adblockplus.org/29636585/diff/29770564/README.md File README.md (right): https://codereview.adblockplus.org/29636585/diff/29770564/README.md#newcode183 README.md:183: | desktop-options.json | options_whitelist_placeholder | Input placeholder prefix | {"domain":{"content":"$1"}} | e.g. $domain$ | | | | On 2018/05/16 17:06:36, saroyanm wrote: > What is the problem with real strings ? What you're trying to do is to outline the capabilities of this format. Keeping the content to a minimum and the example self-contained helps to avoid distractions. Consider this example: | Filename | StringID | Description | Placeholders | en_US | af | am | ... | |--------------|----------|----------------------|-----------------------------|---------------|------------|-----|-----| | options.json | cancel | Cancel button label | | Cancel | Kanselleer | ይቅር | ... | | options.json | domain | Domain input example | {"domain":{"content":"$1"}} | e.g. $domain$ | | | ... | The content is pretty much the same as the real one but without implementation-specifics such as - "desktop-" prefix for the file name - Long string IDs that are necessary to avoid naming conflicts - References to something that cannot be found within the examples (e.g. "Advanced tab") Finally, using real strings means that we'd need to keep them here in sync with the ones in the code which could lead to further confusion in the future. https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:207: console.log(`Updated: ${filePath}`); On 2018/05/04 13:56:27, saroyanm wrote: > console.log doesn't pass eslint -> Unexpected console statement. (no-console) > > This sounds more like an exceptional case for the csv-exporter that we would > like to have those outputs. Agreed, feel free to use "eslint-disable" to ignore this rule for this line. https://codereview.adblockplus.org/29636585/diff/29784598/README.md File README.md (right): https://codereview.adblockplus.org/29636585/diff/29784598/README.md#newcode180 README.md:180: | filename | StringID | Description | Placeholders | en_US | af | am | ... | Detail: The header name here needs to be updated to reflect the change at https://codereview.adblockplus.org/29636585/diff2/29770564:29783609/build/csv... https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:279: {repo} - Name of the "Default" repository Detail: These placeholders no longer exist.
On 2018/05/17 17:27:38, saroyanm wrote: > This is ready for review. > > > CC: @wspee I have removed the logic that were using commit hash to generate > file, as it was mercurial specific, we might still want to think about naming > and implement it separately if we think this is a feature we need. > > https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js > File build/csv-export.js (right): > > https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... > build/csv-export.js:49: mercurialCommands.push(execFile("hg", ["id", "-i"])); > On 2018/05/15 18:59:49, saroyanm wrote: > > This will not work in git. > > > > Alternative: mercurialCommands.push(execFile("git", ["rev-parse", "HEAD"])); > > As discussed: We better remove this mercurial specific logic and introduce it > back if we think we really need it. SGTM it would be nice to have but it doesn't need to be in initially.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:148: if (locale == defaultLocale) 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:148: if (locale == defaultLocale) On 2018/05/24 13:29:26, saroyanm wrote: > 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. 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:172: let sortedJSON = orderJSON(dataTreeObj[fileName][locale]); 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:172: let sortedJSON = orderJSON(dataTreeObj[fileName][locale]); On 2018/05/24 15:29:54, saroyanm wrote: > 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. 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:112: return csvParser(fileObjects); 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:1: /* 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:1: /* On 2018/05/24 17:00:59, saroyanm wrote: > 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. 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.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:1: /* On 2018/05/25 10:23:28, Thomas Greiner wrote: > On 2018/05/24 17:00:59, saroyanm wrote: > > 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. > > 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 ` `. 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 ?
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:1: /* On 2018/05/28 13:37:08, saroyanm wrote: > I wonder why we used code point in the first place ? 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.
https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/05/22 17:22:48, Thomas Greiner wrote: > On 2018/05/16 17:06:32, saroyanm wrote: > > Done. > > You replaced the `reduce()` call with a for-of loop now. It still contains the > same logic as before though, which means that there's still redundant loops for > populating the `locales` and `fileNames` arrays. I don't understand this comment, what is wrong with current implementation ?
I tried to clarify my comment. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/06/04 15:17:59, saroyanm wrote: > I don't understand this comment, what is wrong with current implementation ? The problem I'm referring to is that you're handling each locale and each file name twice: Once in this loop and the second time in `locales.filter()` and `fileNames.filter()` respectively. Consider this question: Why would you add an element to a list that you're going to filter out right afterwards anyway? So instead of filtering elements after you add them, you could simply add a check in this loop before you add them (see code example in original comment).
I think I have everything now. Thanks for the comments. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/06/04 16:58:21, Thomas Greiner wrote: > On 2018/06/04 15:17:59, saroyanm wrote: > > I don't understand this comment, what is wrong with current implementation ? > > The problem I'm referring to is that you're handling each locale and each file > name twice: Once in this loop and the second time in `locales.filter()` and > `fileNames.filter()` respectively. > > Consider this question: Why would you add an element to a list that you're going > to filter out right afterwards anyway? > > So instead of filtering elements after you add them, you could simply add a > check in this loop before you add them (see code example in original comment). Got it, thanks for the clarification, I'll fix that. https://codereview.adblockplus.org/29636585/diff/29770564/README.md File README.md (right): https://codereview.adblockplus.org/29636585/diff/29770564/README.md#newcode183 README.md:183: | desktop-options.json | options_whitelist_placeholder | Input placeholder prefix | {"domain":{"content":"$1"}} | e.g. $domain$ | | | | On 2018/05/22 17:22:49, Thomas Greiner wrote: > On 2018/05/16 17:06:36, saroyanm wrote: > > What is the problem with real strings ? > > What you're trying to do is to outline the capabilities of this format. Keeping > the content to a minimum and the example self-contained helps to avoid > distractions. Consider this example: > > | Filename | StringID | Description | Placeholders | > en_US | af | am | ... | > |--------------|----------|----------------------|-----------------------------|---------------|------------|-----|-----| > | options.json | cancel | Cancel button label | | > Cancel | Kanselleer | ይቅር | ... | > | options.json | domain | Domain input example | {"domain":{"content":"$1"}} | > e.g. $domain$ | | | ... | > > The content is pretty much the same as the real one but without > implementation-specifics such as > - "desktop-" prefix for the file name > - Long string IDs that are necessary to avoid naming conflicts > - References to something that cannot be found within the examples (e.g. > "Advanced tab") I'll use your version, thanks. > Finally, using real strings means that we'd need to keep them here in sync with > the ones in the code which could lead to further confusion in the future. No we wouldn't at least I wasn't planing to keep in sync until it's a structural change.
Thanks Thomas. This is ready for the review. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js File csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:29: const readFile = promisify(fs.readFile); On 2018/05/22 17:22:48, Thomas Greiner wrote: > On 2018/05/16 17:06:33, saroyanm wrote: > > On 2018/05/07 15:16:37, Thomas Greiner wrote: > > > This variable is unused so let's use it instead of `fs.readFile()`. > > > > The variable is used. see, line: 131 > > Still, you're using `fs.readFile()` in `readJson()`. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/06/05 10:35:26, saroyanm wrote: > On 2018/06/04 16:58:21, Thomas Greiner wrote: > > On 2018/06/04 15:17:59, saroyanm wrote: > > > I don't understand this comment, what is wrong with current implementation ? > > > > The problem I'm referring to is that you're handling each locale and each file > > name twice: Once in this loop and the second time in `locales.filter()` and > > `fileNames.filter()` respectively. > > > > Consider this question: Why would you add an element to a list that you're > going > > to filter out right afterwards anyway? > > > > So instead of filtering elements after you add them, you could simply add a > > check in this loop before you add them (see code example in original comment). > > Got it, thanks for the clarification, I'll fix that. Done. https://codereview.adblockplus.org/29636585/diff/29770555/csv-export.js#newco... csv-export.js:193: let orderedJSON = orderJSON(dataTreeObj[fileName][locale]); On 2018/05/22 17:22:47, Thomas Greiner wrote: > On 2018/05/16 17:06:31, saroyanm wrote: > > Done. > > Note that while you renamed the variable, the function is still called > "orderJSON". > > Also note the inconsistency in the capitalization. > > i.e. > - csvFromJsonFileObjects > - orderJSON > - orderedJSON > - readJson > - writeJson > > It'd be great if we could agree on a naming convention for such cases. Done. https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29770564/build/csv-export.js... build/csv-export.js:207: console.log(`Updated: ${filePath}`); On 2018/05/22 17:22:49, Thomas Greiner wrote: > On 2018/05/04 13:56:27, saroyanm wrote: > > console.log doesn't pass eslint -> Unexpected console statement. (no-console) > > > > This sounds more like an exceptional case for the csv-exporter that we would > > like to have those outputs. > > Agreed, feel free to use "eslint-disable" to ignore this rule for this line. Done. https://codereview.adblockplus.org/29636585/diff/29784598/README.md File README.md (right): https://codereview.adblockplus.org/29636585/diff/29784598/README.md#newcode180 README.md:180: | filename | StringID | Description | Placeholders | en_US | af | am | ... | On 2018/05/22 17:22:50, Thomas Greiner wrote: > Detail: The header name here needs to be updated to reflect the change at > https://codereview.adblockplus.org/29636585/diff2/29770564:29783609/build/csv... Done. https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:172: let sortedJSON = orderJSON(dataTreeObj[fileName][locale]); On 2018/05/24 15:46:46, Thomas Greiner wrote: > On 2018/05/24 15:29:54, saroyanm wrote: > > 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. > > 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. I've created a gitlab issue in order to discuss this -> https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/104 https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:279: {repo} - Name of the "Default" repository On 2018/05/22 17:22:50, Thomas Greiner wrote: > Detail: These placeholders no longer exist. Done.
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:79: locales = locales.filter((locale) => locale != defaultLocale).sort(); Is there no need for sorting locales anymore? https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js... build/csv-export.js:228: * @param {function} callback - fileName, locale and strings of locale file Detail: This parameter no longer exists. Instead, you can change the return type to `Promise<Object>` and describe its contents there. https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js... build/csv-export.js:236: return new Promise((resolve, reject) => 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. https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js... build/csv-export.js:273: {hash} - Mercurial current revision hash 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...
https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29784598/build/csv-export.js... build/csv-export.js:79: locales = locales.filter((locale) => locale != defaultLocale).sort(); On 2018/06/12 14:50:41, Thomas Greiner wrote: > Is there no need for sorting locales anymore? If you mean default strings: See -> https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/104
Thanks Thomas. New patch uploaded and ready for review. https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js File build/csv-export.js (right): https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js... build/csv-export.js:228: * @param {function} callback - fileName, locale and strings of locale file On 2018/06/12 14:50:42, Thomas Greiner wrote: > Detail: This parameter no longer exists. Instead, you can change the return type > to `Promise<Object>` and describe its contents there. Done. https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js... build/csv-export.js:236: return new Promise((resolve, reject) => On 2018/06/12 14:50:42, Thomas Greiner wrote: > 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. Done. https://codereview.adblockplus.org/29636585/diff/29799613/build/csv-export.js... build/csv-export.js:273: {hash} - Mercurial current revision hash On 2018/06/12 14:50:42, Thomas Greiner wrote: > 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... Done.
LGTM, only added a small comment https://codereview.adblockplus.org/29636585/diff/29806562/package.json File package.json (right): https://codereview.adblockplus.org/29636585/diff/29806562/package.json#newcode33 package.json:33: "glob": "^7.1.2", Detail: The im-/exporter script isn't involved in building the extension so let's move those two dependencies to "devDependencies".
https://codereview.adblockplus.org/29636585/diff/29806562/package.json File package.json (right): https://codereview.adblockplus.org/29636585/diff/29806562/package.json#newcode33 package.json:33: "glob": "^7.1.2", On 2018/06/15 12:07:20, Thomas Greiner wrote: > Detail: The im-/exporter script isn't involved in building the extension so > let's move those two dependencies to "devDependencies". Good point, done. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
