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

Issue 29465715: Fixes 4969 - Add parsing of filters (Closed)

Created:
June 14, 2017, 5:32 p.m. by Vasily Kuznetsov
Modified:
Oct. 24, 2017, 4:17 p.m.
Reviewers:
mathias
Visibility:
Public.

Description

Fixes 4969 - Add parsing of filters repository: https://hg.adblockplus.org/python-abp base revision: f41b6ebd236a

Patch Set 1 #

Total comments: 14

Patch Set 2 : remove all interpretation and keep only parsing, add support for element hiding emulation filters, … #

Total comments: 17

Patch Set 3 : Address review comments on patch set 2 #

Total comments: 6

Patch Set 4 : Address review comments on patch set 3 #

Total comments: 1

Patch Set 5 : Rebase to 1f5d7ead9bff #

Unified diffs Side-by-side diffs Delta from patch set Stats (+231 lines, -8 lines) Patch
M abp/filters/parser.py View 1 2 3 4 chunks +148 lines, -3 lines 0 comments Download
M tests/test_parser.py View 1 2 3 1 chunk +83 lines, -5 lines 0 comments Download

Messages

Total messages: 17
Vasily Kuznetsov
In the end the implementation changed quite a bit from what I've originally programmed one ...
June 14, 2017, 5:44 p.m. (2017-06-14 17:44:00 UTC) #1
Vasily Kuznetsov
Hi Matze! The conversation in the ticket seems to be resolved enough to move forward ...
July 26, 2017, 2:52 p.m. (2017-07-26 14:52:26 UTC) #2
mathias
https://codereview.adblockplus.org/29465715/diff/29465716/abp/filters/parser.py File abp/filters/parser.py (right): https://codereview.adblockplus.org/29465715/diff/29465716/abp/filters/parser.py#newcode25 abp/filters/parser.py:25: """Internal exception used by the parser to signal invalid ...
July 26, 2017, 8:37 p.m. (2017-07-26 20:37:16 UTC) #3
Vasily Kuznetsov
Hi Matze, Thanks for the comments, I agree with every one of them. Will make ...
July 27, 2017, 11:05 a.m. (2017-07-27 11:05:03 UTC) #4
Vasily Kuznetsov
Oops, looks like I missed these two comments. https://codereview.adblockplus.org/29465715/diff/29465716/setup.py File setup.py (right): https://codereview.adblockplus.org/29465715/diff/29465716/setup.py#newcode46 setup.py:46: version='0.0.2', ...
July 27, 2017, 12:20 p.m. (2017-07-27 12:20:58 UTC) #5
mathias
On 2017/07/27 12:20:58, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29465715/diff/29465716/tests/test_parser.py > File tests/test_parser.py (left): > > https://codereview.adblockplus.org/29465715/diff/29465716/tests/test_parser.py#oldcode80 ...
July 27, 2017, 12:36 p.m. (2017-07-27 12:36:12 UTC) #6
mathias
On 2017/07/27 12:20:58, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29465715/diff/29465716/tests/test_parser.py > File tests/test_parser.py (left): > > https://codereview.adblockplus.org/29465715/diff/29465716/tests/test_parser.py#oldcode80 ...
July 27, 2017, 12:36 p.m. (2017-07-27 12:36:13 UTC) #7
Vasily Kuznetsov
Hi Matze, Here's a more minimalist changeset following your advice. The delta for `parser.py` might ...
July 27, 2017, 7:23 p.m. (2017-07-27 19:23:46 UTC) #8
mathias
I like the simplified patch-set a lot! Just missing some more """info""" in the source. ...
July 28, 2017, 4:43 p.m. (2017-07-28 16:43:30 UTC) #9
Vasily Kuznetsov
Thanks for the comments again, Matze. I just replied to some of your points for ...
July 28, 2017, 5:38 p.m. (2017-07-28 17:38:10 UTC) #10
Vasily Kuznetsov
Addressed all your comments. https://codereview.adblockplus.org/29465715/diff/29499749/abp/filters/parser.py File abp/filters/parser.py (right): https://codereview.adblockplus.org/29465715/diff/29499749/abp/filters/parser.py#newcode38 abp/filters/parser.py:38: class ST: On 2017/07/28 17:38:10, ...
July 28, 2017, 6:57 p.m. (2017-07-28 18:57:49 UTC) #11
mathias
Almost there I think ;) https://codereview.adblockplus.org/29465715/diff/29500688/abp/filters/parser.py File abp/filters/parser.py (right): https://codereview.adblockplus.org/29465715/diff/29500688/abp/filters/parser.py#newcode165 abp/filters/parser.py:165: if name not in ...
Aug. 1, 2017, 6:31 a.m. (2017-08-01 06:31:36 UTC) #12
Vasily Kuznetsov
Hi Matze, Thank you for the comments. Agreed and addressed. Cheers, Vasily https://codereview.adblockplus.org/29465715/diff/29500688/abp/filters/parser.py File abp/filters/parser.py ...
Aug. 2, 2017, 4:21 p.m. (2017-08-02 16:21:18 UTC) #13
mathias
LGTM.
Aug. 2, 2017, 9:54 p.m. (2017-08-02 21:54:33 UTC) #14
Vasily Kuznetsov
FYI, I uploaded the commit after rebasing to the current trunk, so I can be ...
Oct. 24, 2017, 4:03 p.m. (2017-10-24 16:03:31 UTC) #15
mathias
On 2017/10/24 16:03:31, Vasily Kuznetsov wrote: > FYI, I uploaded the commit after rebasing to ...
Oct. 24, 2017, 4:12 p.m. (2017-10-24 16:12:18 UTC) #16
mathias
Oct. 24, 2017, 4:16 p.m. (2017-10-24 16:16:41 UTC) #17
LGTM.

Powered by Google App Engine
This is Rietveld