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

Issue 4515985834901504: Issue 2664 - Remove ignoreHeaders argument from parseFilter(s) and move the logic to the UI (Closed)

Created:
June 8, 2015, 4:39 p.m. by Sebastian Noack
Modified:
June 15, 2015, 7:13 a.m.
Reviewers:
Wladimir Palant
CC:
kzar
Visibility:
Public.

Description

Issue 2664 - Remove ignoreHeaders argument from parseFilter(s) and move the logic to the UI

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Exclude filter list header errors using .forEach() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -62 lines) Patch
M background.js View 1 chunk +2 lines, -2 lines 0 comments Download
M lib/filterValidation.js View 4 chunks +82 lines, -32 lines 0 comments Download
M options.js View 1 1 chunk +8 lines, -3 lines 0 comments Download
M qunit/tests/filterValidation.js View 2 chunks +13 lines, -25 lines 0 comments Download

Messages

Total messages: 4
Sebastian Noack
June 8, 2015, 4:46 p.m. (2015-06-08 16:46:57 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/4515985834901504/diff/5757334940811264/options.js File options.js (right): http://codereview.adblockplus.org/4515985834901504/diff/5757334940811264/options.js#newcode548 options.js:548: } Why not: var errors = result.errors.filter(function(e) { return ...
June 8, 2015, 6:15 p.m. (2015-06-08 18:15:46 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/4515985834901504/diff/5757334940811264/options.js File options.js (right): http://codereview.adblockplus.org/4515985834901504/diff/5757334940811264/options.js#newcode548 options.js:548: } On 2015/06/08 18:15:47, Wladimir Palant wrote: > Why ...
June 8, 2015, 6:36 p.m. (2015-06-08 18:36:46 UTC) #3
Wladimir Palant
June 8, 2015, 7:14 p.m. (2015-06-08 19:14:16 UTC) #4
LGTM

http://codereview.adblockplus.org/4515985834901504/diff/5757334940811264/quni...
File qunit/tests/filterValidation.js (right):

http://codereview.adblockplus.org/4515985834901504/diff/5757334940811264/quni...
qunit/tests/filterValidation.js:19:
ok(/\b4\b/.test(parseFilters("!comment\r\n||example.com^\n\n##/").errors[0]),
"error contains corresponding line number");
On 2015/06/08 18:36:46, Sebastian Noack wrote:
> I considered that. But then decided to keep it as it is to  also test the code
> path adding the line number to the error message. However, I wouldn't mind
> changing it if you disagree.

Well, I'm not going to insist - I merely dislike parsing localizable strings.

Powered by Google App Engine
This is Rietveld