Issue 29328931: Issue 3176 - Keep filter list parameters (Closed)
|
Created:
Oct. 7, 2015, 6:48 p.m. by Felix Dahlke Modified:
Oct. 20, 2015, 4:05 p.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 3176 - Keep filter list parameters
Patch Set 1 : #
Total comments: 4
MessagesTotal messages: 6
https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js#ne... abp2blocklist.js:74: var paramMatch = /^\s*!\s*([\w ]+)\s*:\s*(.*)/.exec(line); I think we should leave the parsing to the core code imported from ABP. So instead using a regex here you could check for CommentFilter objects returned by Filter.fromText(). https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js#ne... abp2blocklist.js:283: console.log("// " + key + ": " + params[key]); But anyway, valid JSON cannot have comments. So NOT LGTM.
https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js#ne... abp2blocklist.js:283: console.log("// " + key + ": " + params[key]); On 2015/10/13 11:25:08, Sebastian Noack wrote: > But anyway, valid JSON cannot have comments. So NOT LGTM. Did you read https://issues.adblockplus.org/ticket/3176? The only alternative I see so far is to return a different JSON structure, i.e. not an array of content blocking rule objects. Stripping out comments is easy, stripping out anything else on the client-side is more involved.
On 2015/10/13 11:37:26, Felix Dahlke wrote: > https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js > File abp2blocklist.js (right): > > https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js#ne... > abp2blocklist.js:283: console.log("// " + key + ": " + params[key]); > On 2015/10/13 11:25:08, Sebastian Noack wrote: > > But anyway, valid JSON cannot have comments. So NOT LGTM. > > Did you read https://issues.adblockplus.org/ticket/3176 > > The only alternative I see so far is to return a different JSON structure, i.e. > not an array of content blocking rule objects. Stripping out comments is easy, > stripping out anything else on the client-side is more involved. Generating invalid JSON certainly isn't the way to go. I consider using non-standard-compliant formats bad practice. Also note that I sometimes load the generated file with other tolls like Python during development to analyze and debug the output. So a better approach would be to either wrap the block list by another data structure, or provide the metadata separately in a separate file or using HTTP headers.
https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js File abp2blocklist.js (right): https://codereview.adblockplus.org/29328931/diff/29328942/abp2blocklist.js#ne... abp2blocklist.js:283: console.log("// " + key + ": " + params[key]); On 2015/10/13 11:37:26, Felix Dahlke wrote: > On 2015/10/13 11:25:08, Sebastian Noack wrote: > > But anyway, valid JSON cannot have comments. So NOT LGTM. > > Did you read https://issues.adblockplus.org/ticket/3176 > > The only alternative I see so far is to return a different JSON structure, i.e. > not an array of content blocking rule objects. Stripping out comments is easy, > stripping out anything else on the client-side is more involved. But well, different approaches shouldn't be discussed here, that should go into https://issues.adblockplus.org/ticket/3176. Please have a look and suggest a different approach there.
Message was sent while issue was closed.
Closed this. After putting some more thought into it, this just doesn't make sense, see: https://issues.adblockplus.org/ticket/3176#comment:19. 
|
||||||||||||||||||||||||||||||||||||||||||
