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

Issue 29884571: Issue 6945 - Add script to make filter list diffs (Closed)

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

Description

Issue 6945 - Add script to make filter list diffs

Patch Set 1 #

Total comments: 11

Patch Set 2 : Use io.open instead of open() #

Total comments: 14

Patch Set 3 : Handle non-ascii, remove extraneous test #

Total comments: 12

Patch Set 4 : Address comments on PS3, add README #

Total comments: 11

Patch Set 5 : Address comments on PS4 #

Total comments: 4

Patch Set 6 : Address comments on PS4 #

Total comments: 2

Patch Set 7 : Fix docstring #

Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -3 lines) Patch
M README.md View 1 2 3 4 5 2 chunks +23 lines, -0 lines 0 comments Download
A abp/filters/diff_script.py View 1 2 3 4 5 1 chunk +60 lines, -0 lines 0 comments Download
M abp/filters/renderer.py View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M setup.py View 1 chunk +2 lines, -1 line 0 comments Download
A tests/test_diff_script.py View 1 2 3 4 5 6 1 chunk +85 lines, -0 lines 0 comments Download
M tests/test_differ.py View 1 2 3 3 chunks +3 lines, -2 lines 0 comments Download

Messages

Total messages: 20
rhowell
1 year, 2 months ago (2018-09-18 15:57:46 UTC) #1
rhowell
Hey Vasily, There are a couple spots that can likely be improved, but I wanted ...
1 year, 2 months ago (2018-09-18 16:01:54 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29884571/diff/29884572/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29884572/abp/filters/diff_script.py#newcode52 abp/filters/diff_script.py:52: latest = open(args.latest, 'r') Please make sure all files ...
1 year, 2 months ago (2018-09-18 16:56:12 UTC) #3
rhowell
Thanks for the quick feedback! https://codereview.adblockplus.org/29884571/diff/29884572/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29884572/abp/filters/diff_script.py#newcode52 abp/filters/diff_script.py:52: latest = open(args.latest, 'r') ...
1 year, 2 months ago (2018-09-18 17:23:33 UTC) #4
Sebastian Noack
I also noticed that in render_script.py we don't read files directly from disk but use ...
1 year, 2 months ago (2018-09-18 19:13:19 UTC) #5
Vasily Kuznetsov
Hi Rosie, I would say that the overall approach looks right to me. I have ...
1 year, 2 months ago (2018-09-19 18:40:04 UTC) #6
Vasily Kuznetsov
On 2018/09/18 19:13:19, Sebastian Noack wrote: > I also noticed that in render_script.py we don't ...
1 year, 2 months ago (2018-09-19 18:42:04 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29884571/diff/29884576/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29884576/abp/filters/diff_script.py#newcode53 abp/filters/diff_script.py:53: lines = render_diff(base, latest) On 2018/09/19 18:40:04, Vasily Kuznetsov ...
1 year, 2 months ago (2018-09-20 16:59:12 UTC) #8
Vasily Kuznetsov
https://codereview.adblockplus.org/29884571/diff/29884576/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29884576/abp/filters/diff_script.py#newcode56 abp/filters/diff_script.py:56: sys.stdout.write(line + '\n') On 2018/09/20 16:59:12, Sebastian Noack wrote: ...
1 year, 2 months ago (2018-09-21 07:48:42 UTC) #9
rhowell
Hey, thanks for the feedback, it was very helpful. Let me know if you see ...
1 year, 2 months ago (2018-09-21 08:37:09 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29884571/diff/29887555/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29887555/abp/filters/diff_script.py#newcode19 abp/filters/diff_script.py:19: import argparse Nit: __future__ imports are special, so I ...
1 year, 2 months ago (2018-09-21 11:51:21 UTC) #11
rhowell
Hey, thanks for the feedback! I've addressed all the comments, and added a REAME. Let ...
1 year, 2 months ago (2018-09-24 22:05:22 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29884571/diff/29890625/README.md File README.md (right): https://codereview.adblockplus.org/29884571/diff/29890625/README.md#newcode91 README.md:91: A diff allows a client running adblocking software such ...
1 year, 2 months ago (2018-09-24 22:21:09 UTC) #13
rhowell
Thanks for the feedback! Let me know if you see any other issues. https://codereview.adblockplus.org/29884571/diff/29890625/README.md File ...
1 year, 2 months ago (2018-09-25 01:53:45 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29884571/diff/29890625/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29890625/abp/filters/diff_script.py#newcode33 abp/filters/diff_script.py:33: nargs='?') On 2018/09/25 01:53:45, rhowell wrote: > On 2018/09/24 ...
1 year, 2 months ago (2018-09-25 17:00:39 UTC) #15
rhowell
https://codereview.adblockplus.org/29884571/diff/29890631/README.md File README.md (right): https://codereview.adblockplus.org/29884571/diff/29890631/README.md#newcode111 README.md:111: The diff is generated such that the removed filters ...
1 year, 2 months ago (2018-09-25 23:01:46 UTC) #16
Sebastian Noack
LGTM https://codereview.adblockplus.org/29884571/diff/29884576/abp/filters/diff_script.py File abp/filters/diff_script.py (right): https://codereview.adblockplus.org/29884571/diff/29884576/abp/filters/diff_script.py#newcode56 abp/filters/diff_script.py:56: sys.stdout.write(line + '\n') On 2018/09/20 16:59:12, Sebastian Noack ...
1 year, 2 months ago (2018-09-26 11:55:47 UTC) #17
Vasily Kuznetsov
Hi Rosie, Everything looks pretty good to me. There's just one nit about the docstring ...
1 year, 2 months ago (2018-09-26 16:50:41 UTC) #18
rhowell
> > I just noticed, we might have the same issue in render_script. This should ...
1 year, 2 months ago (2018-09-26 18:22:57 UTC) #19
Vasily Kuznetsov
1 year, 2 months ago (2018-09-27 10:07:44 UTC) #20
LGTM

On 2018/09/26 18:22:57, rhowell wrote:
> > > I just noticed, we might have the same issue in render_script. This should
> > > probably be addressed in a separate change.
> 
> > Yeah, makes sense. Perhaps we could move this piece of code out and import
> into
> > both scripts.
> 
> I can do this, if you want. Should I make an issue? Or Noissue?

I think a noissue would do. We could extract the code from fldiff into a method
that would be then called like this:

    with open_outfile(args.outfile) as outfile:
        print(..., file=outfile)

and then use it in both scripts. Seems like the description of this change would
fit into a commit message.
Sign in to reply to this message.

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