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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by Sebastian Noack
Modified:
1 year, 1 month ago
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 ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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. ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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*(.*)/` ...
1 year, 1 month ago (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: > > ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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 ...
1 year, 1 month ago (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"]; ...
1 year, 1 month ago (2018-09-10 18:51:11 UTC) #13
Manish Jethani
LGTM
1 year, 1 month ago (2018-09-10 18:52:11 UTC) #14
Manish Jethani
1 year, 1 month ago (2018-09-11 15:12:49 UTC) #15
On 2018/09/10 18:52:11, Manish Jethani wrote:
> LGTM

You can close this now.
Sign in to reply to this message.

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