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

Issue 29845767: Issue 6685 - Offer incremental filter list downloads (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by rhowell
Modified:
3 weeks, 3 days ago
Base URL:
https://hg.adblockplus.org/python-abp/
Visibility:
Public.

Description

Issue 6685 - Offer incremental filter list downloads

Patch Set 1 #

Patch Set 2 : Use sets instead of ndiff, account for removal of special comments #

Patch Set 3 : Use namedtuple filter list objects instead of strings #

Total comments: 13

Patch Set 4 : Use iterables instead of str, stop repeating code #

Total comments: 25

Patch Set 5 : Remove metadata_keys, yield deletions first #

Total comments: 10

Patch Set 6 : Address comments on PS5 #

Patch Set 7 : Store metadata in a dict #

Total comments: 8

Patch Set 8 : Address comments on PS7 #

Patch Set 9 : Address comments on PS8 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+126 lines, -13 lines) Patch
M abp/filters/parser.py View 1 2 3 4 2 chunks +2 lines, -4 lines 0 comments Download
M abp/filters/renderer.py View 1 2 3 4 5 6 7 3 chunks +49 lines, -4 lines 2 comments Download
A tests/test_differ.py View 1 2 3 4 5 6 7 8 1 chunk +75 lines, -0 lines 0 comments Download
M tests/test_parser.py View 1 2 3 4 5 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 26
rhowell
2 months, 2 weeks ago (2018-08-03 01:58:27 UTC) #1
rhowell
On 2018/08/03 01:58:27, rhowell wrote: Hi Vasily! Wanted to check in and get some feedback ...
2 months, 2 weeks ago (2018-08-03 02:03:27 UTC) #2
Vasily Kuznetsov
On 2018/08/03 02:03:27, rhowell wrote: > On 2018/08/03 01:58:27, rhowell wrote: > > Hi Vasily! ...
2 months, 2 weeks ago (2018-08-03 17:39:27 UTC) #3
rhowell
It wasn't possible to use the namedtuple filter list objects in sets, because they contain ...
2 months, 1 week ago (2018-08-09 19:39:02 UTC) #4
Vasily Kuznetsov
Hi Rosie! Sorry for the late reply -- my vacation has started just when you ...
2 months ago (2018-08-17 10:30:54 UTC) #5
rhowell
Hey Vasily! Thanks for the feedback; the structure seems a lot cleaner like this. Just ...
2 months ago (2018-08-20 18:21:27 UTC) #6
Vasily Kuznetsov
LGTM https://codereview.adblockplus.org/29845767/diff/29851647/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29851647/abp/filters/renderer.py#newcode212 abp/filters/renderer.py:212: if line.type == 'metadata' and 'Checksum' not in ...
1 month, 4 weeks ago (2018-08-21 15:00:00 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29851647/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29851647/abp/filters/renderer.py#newcode212 abp/filters/renderer.py:212: if line.type == 'metadata' and 'Checksum' not in line.to_string(): ...
1 month, 4 weeks ago (2018-08-21 19:42:46 UTC) #8
Vasily Kuznetsov
Thanks for the comments, Sebastian! Here are a few comments on the comments :) https://codereview.adblockplus.org/29845767/diff/29859561/abp/filters/parser.py ...
1 month, 4 weeks ago (2018-08-22 13:10:50 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29859561/abp/filters/parser.py File abp/filters/parser.py (right): https://codereview.adblockplus.org/29845767/diff/29859561/abp/filters/parser.py#newcode145 abp/filters/parser.py:145: 'Version', 'Diff-URL', 'Diff-Expires'} On 2018/08/22 13:10:50, Vasily Kuznetsov wrote: ...
1 month, 4 weeks ago (2018-08-22 14:31:06 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29851647/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29851647/abp/filters/renderer.py#newcode212 abp/filters/renderer.py:212: if line.type == 'metadata' and 'Checksum' not in line.to_string(): ...
1 month, 4 weeks ago (2018-08-22 16:03:30 UTC) #11
Vasily Kuznetsov
https://codereview.adblockplus.org/29845767/diff/29859561/abp/filters/parser.py File abp/filters/parser.py (right): https://codereview.adblockplus.org/29845767/diff/29859561/abp/filters/parser.py#newcode156 abp/filters/parser.py:156: if match and match.group(1) in METADATA_KEYS: On 2018/08/22 14:31:06, ...
1 month, 3 weeks ago (2018-08-24 13:03:19 UTC) #12
rhowell
Thanks for the input! I made the suggested changes. Let me know if you see ...
1 month, 3 weeks ago (2018-08-27 22:06:27 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py#newcode127 abp/filters/renderer.py:127: seen = {'Checksum'} Since we parse metadata with arbitrary ...
1 month, 3 weeks ago (2018-08-28 19:52:19 UTC) #14
rhowell
https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py#newcode127 abp/filters/renderer.py:127: seen = {'Checksum'} On 2018/08/28 19:52:18, Sebastian Noack wrote: ...
1 month, 3 weeks ago (2018-08-29 21:43:35 UTC) #15
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py#newcode190 abp/filters/renderer.py:190: keys.add(line.key) On 2018/08/29 21:43:34, rhowell wrote: > On 2018/08/28 ...
1 month, 3 weeks ago (2018-08-29 21:56:37 UTC) #16
rhowell
https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29866656/abp/filters/renderer.py#newcode190 abp/filters/renderer.py:190: keys.add(line.key) On 2018/08/29 21:56:37, Sebastian Noack wrote: > On ...
1 month, 3 weeks ago (2018-08-29 22:52:24 UTC) #17
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py File tests/test_differ.py (right): https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py#newcode21 tests/test_differ.py:21: ! Diff-URL: https://easylist-downloads.adblockplus.org/easylist/diffs/111.txt Perhaps turn one of the keys ...
1 month, 3 weeks ago (2018-08-29 22:58:10 UTC) #18
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py File tests/test_differ.py (right): https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py#newcode21 tests/test_differ.py:21: ! Diff-URL: https://easylist-downloads.adblockplus.org/easylist/diffs/111.txt On 2018/08/29 22:58:10, Sebastian Noack wrote: ...
1 month, 3 weeks ago (2018-08-29 23:03:24 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29868603/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29868603/abp/filters/renderer.py#newcode215 abp/filters/renderer.py:215: base_md, base_rules = _split_list_for_diff(base) Nit: Is it necessary to ...
1 month, 2 weeks ago (2018-08-29 23:46:26 UTC) #20
rhowell
https://codereview.adblockplus.org/29845767/diff/29868603/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29868603/abp/filters/renderer.py#newcode215 abp/filters/renderer.py:215: base_md, base_rules = _split_list_for_diff(base) On 2018/08/29 23:46:26, Sebastian Noack ...
1 month, 2 weeks ago (2018-08-30 16:23:31 UTC) #21
Sebastian Noack
https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py File tests/test_differ.py (right): https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py#newcode21 tests/test_differ.py:21: ! Diff-URL: https://easylist-downloads.adblockplus.org/easylist/diffs/111.txt On 2018/08/29 23:03:24, Sebastian Noack wrote: ...
1 month, 2 weeks ago (2018-08-30 16:38:33 UTC) #22
rhowell
https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py File tests/test_differ.py (right): https://codereview.adblockplus.org/29845767/diff/29868603/tests/test_differ.py#newcode21 tests/test_differ.py:21: ! Diff-URL: https://easylist-downloads.adblockplus.org/easylist/diffs/111.txt On 2018/08/30 16:38:33, Sebastian Noack wrote: ...
1 month, 2 weeks ago (2018-08-30 17:38:20 UTC) #23
Sebastian Noack
LGTM
1 month, 2 weeks ago (2018-08-30 17:42:11 UTC) #24
Vasily Kuznetsov
LGTM again! https://codereview.adblockplus.org/29845767/diff/29869585/abp/filters/renderer.py File abp/filters/renderer.py (right): https://codereview.adblockplus.org/29845767/diff/29869585/abp/filters/renderer.py#newcode187 abp/filters/renderer.py:187: """Split a filter list into metadata, keys, ...
1 month, 2 weeks ago (2018-08-31 12:11:03 UTC) #25
rhowell
1 month, 2 weeks ago (2018-08-31 21:21:03 UTC) #26
https://codereview.adblockplus.org/29845767/diff/29869585/abp/filters/rendere...
File abp/filters/renderer.py (right):

https://codereview.adblockplus.org/29845767/diff/29869585/abp/filters/rendere...
abp/filters/renderer.py:187: """Split a filter list into metadata, keys, and
rules."""
On 2018/08/31 12:11:02, Vasily Kuznetsov wrote:
> Nit: strictly speaking this function now returns metadata and rules (the keys
> are just part of metadata). The docstring could be updated.

Ah, good point! I'll update that.
Sign in to reply to this message.

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