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

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

[Vasily Kuznetsov, 2018-07-09 15:15:45]

Hi Tudor,

For the future, please post some message into the review after you upload it and
set the reviewers. Otherwise they don't get notified.

I will take a look at this one.

Cheers,
Vasily

[Tudor Avram, 2018-07-09 15:26:28]

On 2018/07/09 15:15:45, Vasily Kuznetsov wrote:
> Hi Tudor,
> 
> For the future, please post some message into the review after you upload it
and
> set the reviewers. Otherwise they don't get notified.
> 
> I will take a look at this one.
> 
> Cheers,
> Vasily

Ok, sorry about that! 
Tudor.

[Vasily Kuznetsov, 2018-07-09 17:00:58]

Hi Tudor,

This looks pretty good. I have a couple of nits regarding the README and a
suggestion for making the tests a bit shorter and less repetitive (see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29824555/diff/29824556/README.md
File README.md (right):

https://codereview.adblockplus.org/29824555/diff/29824556/README.md#newcode45
README.md:45: in which case the rendering result will be returned to `stdout`.
Moreover, when
"returned" feels a bit weird here. What do you think about "will be _sent_ to
stdout"?

https://codereview.adblockplus.org/29824555/diff/29824556/README.md#newcode46
README.md:46: being run with no positional arguments:
Perhaps eliminate the passive here to make it easier to read? "Moreover, if you
run `flrender` with no ..."

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

https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
tests/test_render_script.py:97: if 'test_in'in kw.keys():
Space missing here (weird that flake8 is ok...)

https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
tests/test_render_script.py:135: def test_stdin_stdout(rootdir):
Wouldn't it be simpler to model this and the following test on
test_render_no_includes:
- We don't need -i and -v options here.
- We can just read test input from simple.txt (something like
rootdir.read('simple.txt') should do the trick).
- Use the same checks as test_render_no_includes.

Actually you might even parametrize test_render_no_includes to work in three
modes: input from file + output to file, input from file + output to stdout,
input from stdin + output to stdout and then reuse all similar code.

[Tudor Avram, 2018-07-10 08:06:21]

Got it! I have a question, though (see below).

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

https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
tests/test_render_script.py:135: def test_stdin_stdout(rootdir):
On 2018/07/09 17:00:57, Vasily Kuznetsov wrote:
> Wouldn't it be simpler to model this and the following test on
> test_render_no_includes:
> - We don't need -i and -v options here.
> - We can just read test input from simple.txt (something like
> rootdir.read('simple.txt') should do the trick).
> - Use the same checks as test_render_no_includes.
> 
> Actually you might even parametrize test_render_no_includes to work in three
> modes: input from file + output to file, input from file + output to stdout,
> input from stdin + output to stdout and then reuse all similar code.

Ok, will do that. From what I see, the case for input from file + output to file
is already handled by test_render_with_includes(), test_render_unicode() and
test_render_with_includes_relative(). Does it make sense to include it as a test
here as well?

[Tudor Avram, 2018-07-10 08:06:21]

Got it! I have a question, though (see below).

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

https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
tests/test_render_script.py:135: def test_stdin_stdout(rootdir):
On 2018/07/09 17:00:57, Vasily Kuznetsov wrote:
> Wouldn't it be simpler to model this and the following test on
> test_render_no_includes:
> - We don't need -i and -v options here.
> - We can just read test input from simple.txt (something like
> rootdir.read('simple.txt') should do the trick).
> - Use the same checks as test_render_no_includes.
> 
> Actually you might even parametrize test_render_no_includes to work in three
> modes: input from file + output to file, input from file + output to stdout,
> input from stdin + output to stdout and then reuse all similar code.

Ok, will do that. From what I see, the case for input from file + output to file
is already handled by test_render_with_includes(), test_render_unicode() and
test_render_with_includes_relative(). Does it make sense to include it as a test
here as well?

[Vasily Kuznetsov, 2018-07-10 14:39:49]

On 2018/07/10 08:06:21, Tudor Avram wrote:
> Got it! I have a question, though (see below).
> 
>
https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
> File tests/test_render_script.py (right):
> 
>
https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
> tests/test_render_script.py:135: def test_stdin_stdout(rootdir):
> On 2018/07/09 17:00:57, Vasily Kuznetsov wrote:
> > Wouldn't it be simpler to model this and the following test on
> > test_render_no_includes:
> > - We don't need -i and -v options here.
> > - We can just read test input from simple.txt (something like
> > rootdir.read('simple.txt') should do the trick).
> > - Use the same checks as test_render_no_includes.
> > 
> > Actually you might even parametrize test_render_no_includes to work in three
> > modes: input from file + output to file, input from file + output to stdout,
> > input from stdin + output to stdout and then reuse all similar code.
> 
> Ok, will do that. From what I see, the case for input from file + output to
file
> is already handled by test_render_with_includes(), test_render_unicode() and
> test_render_with_includes_relative(). Does it make sense to include it as a
test
> here as well?

At this point testing the file-to-file scenario in test_render_no_includes()
probably doesn't add much value, but having this simple test was useful for me
when I was building the original functionality. It might also be useful in the
future to see what exactly has broken. Technically, if both
test_render_unicode() and test_render_with_includes_relative() break but the
file-to-stdout and stdin-to-stdout modes of test_render_no_includes() pass, we
can suspect that actually file-to-file rendering is broken. But i think having a
separate test for basic file-to-file rendering is a nice clean way to see this
and it's not much extra work when you already have the other two modes done. So
my preference would be to leave it in, but it's not a very strong preference.

[Tudor Avram, 2018-07-10 16:09:47]

Hi Vasily,

I amended the code with your comments. 

Thanks,
Tudor.

https://codereview.adblockplus.org/29824555/diff/29824556/README.md
File README.md (right):

https://codereview.adblockplus.org/29824555/diff/29824556/README.md#newcode45
README.md:45: in which case the rendering result will be returned to `stdout`.
Moreover, when
On 2018/07/09 17:00:57, Vasily Kuznetsov wrote:
> "returned" feels a bit weird here. What do you think about "will be _sent_ to
> stdout"?

Done.

https://codereview.adblockplus.org/29824555/diff/29824556/README.md#newcode46
README.md:46: being run with no positional arguments:
On 2018/07/09 17:00:57, Vasily Kuznetsov wrote:
> Perhaps eliminate the passive here to make it easier to read? "Moreover, if
you
> run `flrender` with no ..."

Done.

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

https://codereview.adblockplus.org/29824555/diff/29824556/tests/test_render_s...
tests/test_render_script.py:97: if 'test_in'in kw.keys():
On 2018/07/09 17:00:57, Vasily Kuznetsov wrote:
> Space missing here (weird that flake8 is ok...)

Done.

[Vasily Kuznetsov, 2018-07-13 17:41:04]

Hi Tudor,

Good stuff! I just have a few cosmetic suggestions but otherwise this is done.

Cheers,
Vasily

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

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:99: test_in = kw.pop('test_in')
I just found that you can replace this and the two previous lines with:

    test_in = kw.pop('test_in', None)

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:111: ('None', ("'simple.txt'", 'str(dstfile)')),
We iterate over the inner tuple, so according to our style guide [1] it should
be a list (same for the next line). Kind of minor but just to make it more...
neat :)

[1] https://adblockplus.org/coding-style#python

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:117: test_input = eval(test_input, _locals)
But this eval doesn't need the _locals, right? It should be able to see them by
itself because it's in the right scope. Or am I wrong?

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:118: args = [eval(arg, _locals) for arg in args]
It seems that if you use map(eval, args) instead of the list comprehension, you
don't need to pass the locals. Unfortunately in Python3 the result of the map
can't be the argument of len() but you can still do list(map(eval, args)) and I
think it's still nicer.

[Tudor Avram, 2018-07-16 09:17:12]

Hi Vasily, 

I addressed your comments form the previous patch. On top of that, I updated
test_render_no_includes() to check for the '! Checksum:' string, as well. It was
making this check beforehand and when I re-wrote the function I forgot to
include it. 

Thanks, 
Tudor.

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

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:99: test_in = kw.pop('test_in')
On 2018/07/13 17:41:04, Vasily Kuznetsov wrote:
> I just found that you can replace this and the two previous lines with:
> 
>     test_in = kw.pop('test_in', None)

Done.

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:117: test_input = eval(test_input, _locals)
On 2018/07/13 17:41:04, Vasily Kuznetsov wrote:
> But this eval doesn't need the _locals, right? It should be able to see them
by
> itself because it's in the right scope. Or am I wrong?

Done.

https://codereview.adblockplus.org/29824555/diff/29826592/tests/test_render_s...
tests/test_render_script.py:118: args = [eval(arg, _locals) for arg in args]
On 2018/07/13 17:41:04, Vasily Kuznetsov wrote:
> It seems that if you use map(eval, args) instead of the list comprehension,
you
> don't need to pass the locals. Unfortunately in Python3 the result of the map
> can't be the argument of len() but you can still do list(map(eval, args)) and
I
> think it's still nicer.

Done.

[Vasily Kuznetsov, 2018-07-17 09:26:59]

Hey!

All looks great but you forgot that _locals (it's no longer needed).

Cheers,
Vasily

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()
Nit: this variable is not used now

[Tudor Avram, 2018-07-17 09:40:08]

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.