Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29873570: Issue 6923 - Only parse metadata from special comments at the top of the file (Closed)

Created:
Sept. 3, 2018, 6:43 p.m. by Sebastian Noack
Modified:
Sept. 11, 2018, 5:23 p.m.
Reviewers:
Manish Jethani
Visibility:
Public.

Description

Issue 6923 - Only parse metadata from special comments at the top of the file

Patch Set 1 #

Total comments: 9

Patch Set 2 : Change end-of-metadata semantics #

Total comments: 8

Patch Set 3 : Use more obscure regular expression #

Patch Set 4 : Reverted regexp #

Total comments: 2

Patch Set 5 : Rebased #

Patch Set 6 : Addressed comment about tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -10 lines) Patch
M lib/synchronizer.js View 1 2 3 4 1 chunk +8 lines, -6 lines 0 comments Download
M test/synchronizer.js View 1 2 3 4 5 4 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 15
Sebastian Noack
https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let match = /^\s*!\s*(\S.*?)\s*:\s*(.*)/.exec(lines[i]); You might wonder why I ...
Sept. 3, 2018, 6:47 p.m. (2018-09-03 18:47:13 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let match = /^\s*!\s*(\S.*?)\s*:\s*(.*)/.exec(lines[i]); On 2018/09/03 18:47:13, Sebastian Noack ...
Sept. 4, 2018, 7:14 a.m. (2018-09-04 07:14:42 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js#newcode170 lib/synchronizer.js:170: if (keyword in params) On 2018/09/04 07:14:41, Manish Jethani ...
Sept. 4, 2018, 2:22 p.m. (2018-09-04 14:22:06 UTC) #3
Manish Jethani
LGTM https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js#newcode170 lib/synchronizer.js:170: if (keyword in params) On 2018/09/04 14:22:06, Sebastian ...
Sept. 4, 2018, 3:01 p.m. (2018-09-04 15:01:45 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29873571/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let match = /^\s*!\s*(\S.*?)\s*:\s*(.*)/.exec(lines[i]); On 2018/09/04 07:14:41, Manish Jethani ...
Sept. 4, 2018, 3:32 p.m. (2018-09-04 15:32:39 UTC) #5
Sebastian Noack
Vasily and I agreed to simplify the the condition after which metadata aren't parsed anymore. ...
Sept. 4, 2018, 8:17 p.m. (2018-09-04 20:17:52 UTC) #6
Manish Jethani
On 2018/09/04 20:17:52, Sebastian Noack wrote: > Vasily and I agreed to simplify the the ...
Sept. 5, 2018, 5:39 p.m. (2018-09-05 17:39:36 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let match = /^\s*!\s*(?:(\w+)\s*:\s*(.*)|\S)/.exec(lines[i]); How about this one: `/^\s*!\s*([^\s:]*)\s*(:)?\s*(.*)/` ...
Sept. 5, 2018, 6:10 p.m. (2018-09-05 18:10:09 UTC) #8
Sebastian Noack
On 2018/09/05 17:39:36, Manish Jethani wrote: > On 2018/09/04 20:17:52, Sebastian Noack wrote: > > ...
Sept. 6, 2018, 12:56 a.m. (2018-09-06 00:56:03 UTC) #9
Manish Jethani
https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let match = /^\s*!\s*(?:(\w+)\s*:\s*(.*)|\S)/.exec(lines[i]); On 2018/09/06 00:56:03, Sebastian Noack ...
Sept. 6, 2018, 5:53 a.m. (2018-09-06 05:53:54 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let match = /^\s*!\s*(?:(\w+)\s*:\s*(.*)|\S)/.exec(lines[i]); On 2018/09/06 05:53:53, Manish Jethani ...
Sept. 6, 2018, 2:31 p.m. (2018-09-06 14:31:36 UTC) #11
Manish Jethani
LGTM, except the one comment about the tests https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js File lib/synchronizer.js (right): https://codereview.adblockplus.org/29873570/diff/29874655/lib/synchronizer.js#newcode165 lib/synchronizer.js:165: let ...
Sept. 10, 2018, 7:24 a.m. (2018-09-10 07:24:50 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29873570/diff/29876565/test/synchronizer.js File test/synchronizer.js (left): https://codereview.adblockplus.org/29873570/diff/29876565/test/synchronizer.js#oldcode262 test/synchronizer.js:262: return [Cr.NS_OK, 200, "[Adblock]\nfoo\n!Expires: " + currentTest.expiration + "\nbar"]; ...
Sept. 10, 2018, 6:51 p.m. (2018-09-10 18:51:11 UTC) #13
Manish Jethani
LGTM
Sept. 10, 2018, 6:52 p.m. (2018-09-10 18:52:11 UTC) #14
Manish Jethani
Sept. 11, 2018, 3:12 p.m. (2018-09-11 15:12:49 UTC) #15
On 2018/09/10 18:52:11, Manish Jethani wrote:
> LGTM

You can close this now.

Powered by Google App Engine
This is Rietveld