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

Issue 29824555: Issue #4116: Make infile and outfile parameters of flrender script from python-abp optional (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by Tudor Avram
Modified:
2 months ago
Reviewers:
Vasily Kuznetsov
Visibility:
Public.

Description

Issue #4116: Make infile and outfile parameters of flrender script from python-abp optional

Patch Set 1 #

Total comments: 8

Patch Set 2 : Updated tests and README.md #

Total comments: 7

Patch Set 3 : Addressed comments from patch #2 #

Total comments: 2

Patch Set 4 : Fixed minor flake8 issue #

Patch Set 5 : Removed unnecessary locals() call #

Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -26 lines) Patch
M .gitignore View 1 chunk +2 lines, -0 lines 0 comments Download
M README.md View 1 1 chunk +11 lines, -0 lines 0 comments Download
M abp/filters/render_script.py View 3 chunks +9 lines, -5 lines 0 comments Download
M abp/filters/sources.py View 2 chunks +24 lines, -0 lines 0 comments Download
M tests/test_render_script.py View 1 2 3 4 6 chunks +41 lines, -21 lines 0 comments Download

Messages

Total messages: 11
Vasily Kuznetsov
Hi Tudor, For the future, please post some message into the review after you upload ...
2 months, 1 week ago (2018-07-09 15:15:45 UTC) #1
Tudor Avram
On 2018/07/09 15:15:45, Vasily Kuznetsov wrote: > Hi Tudor, > > For the future, please ...
2 months, 1 week ago (2018-07-09 15:26:28 UTC) #2
Vasily Kuznetsov
Hi Tudor, This looks pretty good. I have a couple of nits regarding the README ...
2 months, 1 week ago (2018-07-09 17:00:58 UTC) #3
Tudor Avram
Got it! I have a question, though (see below). https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_script.py File tests/test_render_script.py (right): https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_script.py#newcode135 tests/test_render_script.py:135: ...
2 months, 1 week ago (2018-07-10 08:06:21 UTC) #4
Tudor Avram
Got it! I have a question, though (see below). https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_script.py File tests/test_render_script.py (right): https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_script.py#newcode135 tests/test_render_script.py:135: ...
2 months, 1 week ago (2018-07-10 08:06:21 UTC) #5
Vasily Kuznetsov
On 2018/07/10 08:06:21, Tudor Avram wrote: > Got it! I have a question, though (see ...
2 months, 1 week ago (2018-07-10 14:39:49 UTC) #6
Tudor Avram
Hi Vasily, I amended the code with your comments. Thanks, Tudor. https://codereview.adblockplus.org/29824555/diff/29824556/README.md File README.md (right): ...
2 months, 1 week ago (2018-07-10 16:09:47 UTC) #7
Vasily Kuznetsov
Hi Tudor, Good stuff! I just have a few cosmetic suggestions but otherwise this is ...
2 months, 1 week ago (2018-07-13 17:41:04 UTC) #8
Tudor Avram
Hi Vasily, I addressed your comments form the previous patch. On top of that, I ...
2 months ago (2018-07-16 09:17:12 UTC) #9
Vasily Kuznetsov
Hey! All looks great but you forgot that _locals (it's no longer needed). Cheers, Vasily ...
2 months ago (2018-07-17 09:26:59 UTC) #10
Tudor Avram
2 months ago (2018-07-17 09:40:08 UTC) #11
Done!

Thanks,
Tudor.

https://codereview.adblockplus.org/29824555/diff/29831555/tests/test_render_s...
File tests/test_render_script.py (right):

https://codereview.adblockplus.org/29824555/diff/29831555/tests/test_render_s...
tests/test_render_script.py:114: _locals = locals()
On 2018/07/17 09:26:58, Vasily Kuznetsov wrote:
> Nit: this variable is not used now

Done.
Sign in to reply to this message.

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