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

Issue 29922555: Issue 7059 - Modify fldiff so it can handle multiple files (Closed)

Created:
Oct. 24, 2018, 2:27 a.m. by rhowell
Modified:
Oct. 31, 2018, 9:14 p.m.
Reviewers:
Vasily Kuznetsov
Base URL:
https://hg.adblockplus.org/python-abp
Visibility:
Public.

Description

Issue 7059 - Modify fldiff so it can handle multiple files

Patch Set 1 #

Total comments: 31

Patch Set 2 : Address comments on PS1 #

Total comments: 9

Patch Set 3 : Address comments on PS2 #

Total comments: 6

Patch Set 4 : Address comments on PS3 #

Total comments: 4

Patch Set 5 : Address comments on PS4 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -57 lines) Patch
M README.md View 1 2 1 chunk +23 lines, -13 lines 0 comments Download
M abp/filters/diff_script.py View 1 2 3 1 chunk +31 lines, -19 lines 0 comments Download
M tests/test_diff_script.py View 1 2 3 4 2 chunks +80 lines, -25 lines 0 comments Download
M tests/test_differ.py View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 13
rhowell
Oct. 24, 2018, 2:27 a.m. (2018-10-24 02:27:34 UTC) #1
rhowell
I left the test with the dictionary in there for now. I'll remove it before ...
Oct. 24, 2018, 2:30 a.m. (2018-10-24 02:30:44 UTC) #2
Vasily Kuznetsov
Hi Rosie, The overall direction looks good but I have quite a few comments and ...
Oct. 24, 2018, 11:30 a.m. (2018-10-24 11:30:10 UTC) #3
rhowell
Hey Vasily! Thanks for the feedback. One quick note: at some point I can change ...
Oct. 26, 2018, 9:42 p.m. (2018-10-26 21:42:23 UTC) #4
Vasily Kuznetsov
Hi Rose! Great progress, this is almost done! I think we also need a test ...
Oct. 29, 2018, 4:31 p.m. (2018-10-29 16:31:49 UTC) #5
rhowell
Hey Vasily! I added the test with no outfile specified, good idea. I already had ...
Oct. 29, 2018, 8:01 p.m. (2018-10-29 20:01:40 UTC) #6
Vasily Kuznetsov
Hi Rosie! Pretty good. There are a couple small nits in the comments below, and ...
Oct. 29, 2018, 11:47 p.m. (2018-10-29 23:47:34 UTC) #7
rhowell
Hey Vasily! I removed the test with the dictionary and the related code, and added ...
Oct. 31, 2018, 12:20 a.m. (2018-10-31 00:20:59 UTC) #8
Vasily Kuznetsov
Hi Rosie! Just a couple more small things. Cheers, Vasily https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_script.py File tests/test_diff_script.py (right): https://codereview.adblockplus.org/29922555/diff/29931578/tests/test_diff_script.py#newcode31 ...
Oct. 31, 2018, 2:42 p.m. (2018-10-31 14:42:40 UTC) #9
Vasily Kuznetsov
On 2018/10/31 00:20:59, rhowell wrote: > > Oh, and did we want to make the ...
Oct. 31, 2018, 2:43 p.m. (2018-10-31 14:43:56 UTC) #10
rhowell
Thanks for the feedback, Vasily! Sounds good about the configurability, if that feature is wanted ...
Oct. 31, 2018, 7:11 p.m. (2018-10-31 19:11:46 UTC) #11
Vasily Kuznetsov
Hi Rosie! LGTM from my side. Maybe get Paco to look at the command line ...
Oct. 31, 2018, 8:28 p.m. (2018-10-31 20:28:58 UTC) #12
rhowell
Oct. 31, 2018, 9:08 p.m. (2018-10-31 21:08:22 UTC) #13
> Maybe get Paco to look at the command line API and check if he's happy with
what
> we have now.
He said it looks good. :)
 
> P.S. It seems that the latest version of flake8 started giving errors in
> python-abp but it doesn't seem like this is your fault so we should address
this
> in a separate ticket.
You saw that too? I thought it was just me! Haven't had a chance to look into it
yet. But, I think it should be fine to push this for now anyway. I'll go push it
now.  :)

Powered by Google App Engine
This is Rietveld