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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 1 week ago by saroyanm
Modified:
3 days, 13 hours ago
CC:
wspee
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: 42

Patch Set 6 : Moved the script into build directory and updated the Readme #

Total comments: 8

Patch Set 7 : Addressed Thomas comments #

Patch Set 8 : Removed mercurial commands #

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

Messages

Total messages: 14
saroyanm
@Thomas this is ready for the review, let's have a look after the Christmas holidays. ...
5 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 ...
3 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 ...
2 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 ...
2 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: ...
2 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 ...
2 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/>, ...
3 weeks, 3 days 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 ...
2 weeks, 2 days 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 ...
2 weeks, 2 days ago (2018-05-04 13:56:28 UTC) #9
juliandoucette
Adding myself to reviewers for future reference.
1 week, 6 days 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: > ...
1 week, 6 days 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 ...
5 days, 12 hours 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 days, 14 hours ago (2018-05-16 17:06:37 UTC) #13
saroyanm
3 days, 13 hours ago (2018-05-17 17:27:38 UTC) #14
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.
Sign in to reply to this message.

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