Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(217)

Issue 29636585: Issue 6171 - create CSV exporter and importer for translations (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 1 week ago by saroyanm
Modified:
1 week, 5 days ago
Visibility:
Public.

Description

Use 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 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -0 lines) Patch
M README.md View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -0 lines 0 comments Download
A build/csv-export.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +309 lines, -0 lines 0 comments Download
M package.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 34
saroyanm
@Thomas this is ready for the review, let's have a look after the Christmas holidays. ...
9 months ago (2017-12-19 19:48:24 UTC) #1
Thomas Greiner
This requires quite a significant amount of changes. One of the reasons for that is ...
7 months, 4 weeks ago (2018-01-22 19:49:55 UTC) #2
saroyanm
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#newcode24 csv-export.js:24: let filesNames = []; // ex.: desktop-options.json On 2018/01/22 ...
6 months, 3 weeks ago (2018-02-28 20:48:33 UTC) #3
saroyanm
Thanks Thomas for detailed review, this is already good enough I think for the second ...
6 months, 3 weeks ago (2018-02-28 20:57:34 UTC) #4
Thomas Greiner
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#newcode27 csv-export.js:27: let outputFileName = "translations-{repo}-{hash}.csv"; On 2018/02/28 20:48:22, saroyanm wrote: ...
6 months ago (2018-03-19 18:28:05 UTC) #5
Thomas Greiner
Finished reviewing the remaining parts of the code I didn't get around to before earlier ...
6 months ago (2018-03-19 18:54:13 UTC) #6
saroyanm
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/>, ...
4 months, 3 weeks ago (2018-04-26 17:53:53 UTC) #7
saroyanm
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 ...
4 months, 2 weeks ago (2018-05-04 13:51:12 UTC) #8
saroyanm
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#newcode207 build/csv-export.js:207: console.log(`Updated: ${filePath}`); console.log doesn't pass eslint -> Unexpected console ...
4 months, 2 weeks ago (2018-05-04 13:56:28 UTC) #9
juliandoucette
Adding myself to reviewers for future reference.
4 months, 2 weeks ago (2018-05-07 11:16:21 UTC) #10
Thomas Greiner
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#newcode58 csv-export.js:58: return Promise.all([readDir(path.join(localesDir, defaultLocale)), On 2018/05/04 13:51:08, saroyanm wrote: > ...
4 months, 2 weeks ago (2018-05-07 15:16:39 UTC) #11
saroyanm
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#newcode31 csv-export.js:31: const readJsonPromised = promisify(readJson); On 2018/05/07 15:16:36, Thomas Greiner ...
4 months ago (2018-05-15 18:59:50 UTC) #12
saroyanm
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#newcode18 csv-export.js:18: /* globals require, process */ On 2018/05/07 15:16:33, Thomas ...
4 months ago (2018-05-16 17:06:37 UTC) #13
saroyanm
This is ready for review. CC: @wspee I have removed the logic that were using ...
4 months ago (2018-05-17 17:27:38 UTC) #14
Thomas Greiner
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#newcode29 csv-export.js:29: const readFile = promisify(fs.readFile); On 2018/05/16 17:06:33, saroyanm wrote: ...
4 months ago (2018-05-22 17:22:51 UTC) #15
wspee
On 2018/05/17 17:27:38, saroyanm wrote: > This is ready for review. > > > CC: ...
3 months, 4 weeks ago (2018-05-23 12:39:00 UTC) #16
saroyanm
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#newcode148 build/csv-export.js:148: if (locale == defaultLocale) I've changed this into if ...
3 months, 4 weeks ago (2018-05-24 13:29:26 UTC) #17
Thomas Greiner
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#newcode148 build/csv-export.js:148: if (locale == defaultLocale) On 2018/05/24 13:29:26, saroyanm wrote: ...
3 months, 4 weeks ago (2018-05-24 13:49:44 UTC) #18
saroyanm
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#newcode172 build/csv-export.js:172: let sortedJSON = orderJSON(dataTreeObj[fileName][locale]); Sorting the Default locale produce ...
3 months, 4 weeks ago (2018-05-24 15:29:54 UTC) #19
Thomas Greiner
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#newcode172 build/csv-export.js:172: let sortedJSON = orderJSON(dataTreeObj[fileName][locale]); On 2018/05/24 15:29:54, saroyanm wrote: ...
3 months, 4 weeks ago (2018-05-24 15:46:47 UTC) #20
saroyanm
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#newcode112 build/csv-export.js:112: return csvParser(fileObjects); I think there might a bug in ...
3 months, 4 weeks ago (2018-05-24 16:40:03 UTC) #21
saroyanm
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#newcode1 build/csv-export.js:1: /* In some stage of import/export This "\u00A0" special ...
3 months, 4 weeks ago (2018-05-24 17:01:00 UTC) #22
Thomas Greiner
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#newcode1 build/csv-export.js:1: /* On 2018/05/24 17:00:59, saroyanm wrote: > In some ...
3 months, 3 weeks ago (2018-05-25 10:23:29 UTC) #23
saroyanm
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#newcode1 build/csv-export.js:1: /* On 2018/05/25 10:23:28, Thomas Greiner wrote: > On ...
3 months, 3 weeks ago (2018-05-28 13:37:09 UTC) #24
Thomas Greiner
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#newcode1 build/csv-export.js:1: /* On 2018/05/28 13:37:08, saroyanm wrote: > I wonder ...
3 months, 3 weeks ago (2018-05-28 17:12:47 UTC) #25
saroyanm
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#newcode79 csv-export.js:79: let dataTreeObj = fileObjects.reduce((accumulator, fileObject) => On 2018/05/22 17:22:48, ...
3 months, 2 weeks ago (2018-06-04 15:18:00 UTC) #26
Thomas Greiner
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#newcode79 csv-export.js:79: let dataTreeObj = ...
3 months, 2 weeks ago (2018-06-04 16:58:22 UTC) #27
saroyanm
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#newcode79 ...
3 months, 2 weeks ago (2018-06-05 10:35:27 UTC) #28
saroyanm
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#newcode29 csv-export.js:29: const ...
3 months, 2 weeks ago (2018-06-05 15:03:48 UTC) #29
Thomas Greiner
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#newcode79 build/csv-export.js:79: locales = locales.filter((locale) => locale != defaultLocale).sort(); Is there ...
3 months, 1 week ago (2018-06-12 14:50:43 UTC) #30
saroyanm
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#newcode79 build/csv-export.js:79: locales = locales.filter((locale) => locale != defaultLocale).sort(); On 2018/06/12 ...
3 months, 1 week ago (2018-06-12 15:22:50 UTC) #31
saroyanm
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#newcode228 build/csv-export.js:228: ...
3 months, 1 week ago (2018-06-13 15:36:27 UTC) #32
Thomas Greiner
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: ...
3 months ago (2018-06-15 12:07:21 UTC) #33
saroyanm
3 months ago (2018-06-18 14:50:52 UTC) #34
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5