Issue 6835 - Add R support to python-abp

Issue 6835 - Add R support to python-abp

[Tudor Avram, 2018-08-07 15:12:08]

Hi all,

Here is the first (basic) implementation of the rPython layer from python-abp. 

Currently, it does not change the datatypes resulted from parsing. It simply
converts the namedtuple to a dict. Going to have a discussion with @kirill after
he comes back from vacation on whether his scripts require slightly different
datatypes.

Thanks and looking forward for your feedback.

Tudor.

[Vasily Kuznetsov, 2018-08-08 14:32:50]

Hi Tudor,

It looks good in general. Perhaps we can add another function for converting a
bunch of text to dicts or an iterable of strings to an iterable of dicts -- we
should ask Kirill what his use case is and make it the most convenient for him.

As for testing strategy, I wonder if just having a simple test that checks some
parsed filter list lines against expected dicts would be sufficient (see my
comment for more detail).

Cheers,
Vasily

https://codereview.adblockplus.org/29849585/diff/29849586/abp/filters/rpy.py
File abp/filters/rpy.py (right):

https://codereview.adblockplus.org/29849585/diff/29849586/abp/filters/rpy.py#...
abp/filters/rpy.py:34: filter: namedtuple
Nit: parameter name mismatch :)

https://codereview.adblockplus.org/29849585/diff/29849586/tests/test_rpy.py
File tests/test_rpy.py (right):

https://codereview.adblockplus.org/29849585/diff/29849586/tests/test_rpy.py#n...
tests/test_rpy.py:61: def check_correct_datatypes(data, expected_types):
What do you think about checking generated dicts against expected dicts with a
slightly more detailed checker than == ? We would just need to also make sure
that the types of all values are the same (otherwise unicode strings might
sometimes look == to byte strings). It seems that this would achieve the same
level of verification but it would have less code and would be easier to follow
and change in the future.

[Tudor Avram, 2018-08-09 12:55:02]

Hi Vasily,

I addressed your comments and refactores test_rpy.py. 

Thanks, 
Tudor.

https://codereview.adblockplus.org/29849585/diff/29849586/abp/filters/rpy.py
File abp/filters/rpy.py (right):

https://codereview.adblockplus.org/29849585/diff/29849586/abp/filters/rpy.py#...
abp/filters/rpy.py:34: filter: namedtuple
On 2018/08/08 14:32:50, Vasily Kuznetsov wrote:
> Nit: parameter name mismatch :)

Done.

https://codereview.adblockplus.org/29849585/diff/29849586/tests/test_rpy.py
File tests/test_rpy.py (right):

https://codereview.adblockplus.org/29849585/diff/29849586/tests/test_rpy.py#n...
tests/test_rpy.py:61: def check_correct_datatypes(data, expected_types):
On 2018/08/08 14:32:50, Vasily Kuznetsov wrote:
> What do you think about checking generated dicts against expected dicts with a
> slightly more detailed checker than == ? We would just need to also make sure
> that the types of all values are the same (otherwise unicode strings might
> sometimes look == to byte strings). It seems that this would achieve the same
> level of verification but it would have less code and would be easier to
follow
> and change in the future.

Done.

[Vasily Kuznetsov, 2018-08-16 16:00:52]

LGTM so far (there's a minor comment about the tests, but it's too minor for
non-LGTM-ing :D)

https://codereview.adblockplus.org/29849585/diff/29851638/tests/test_rpy.py
File tests/test_rpy.py (right):

https://codereview.adblockplus.org/29849585/diff/29851638/tests/test_rpy.py#n...
tests/test_rpy.py:129: @pytest.mark.parametrize('line_type',
list(_TEST_EXAMPLES.keys()))
Do we need the `list(...)` here? If yes, never mind.

[Tudor Avram, 2018-08-20 14:56:29]

Hi Vasily,

Thanks, just did the last change you suggested in the tests.

Tudor.

[Vasily Kuznetsov, 2018-08-21 15:02:33]

On 2018/08/20 14:56:29, Tudor Avram wrote:
> Hi Vasily,
> 
> Thanks, just did the last change you suggested in the tests.
> 
> Tudor.

still LGTM

[Tudor Avram, 2018-08-29 11:47:09]

Hi Vasily and Kirill,

I made the changes to README.md to document the R integration and now this patch
should be good to go.

Thanks, 
Tudor.

[Vasily Kuznetsov, 2018-08-30 12:23:19]

Hi Tudor and Kirill,

I like the addition to the documentation and just have two small stylistic nits
(see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29849585/diff/29868573/README.md
File README.md (right):

https://codereview.adblockplus.org/29849585/diff/29868573/README.md#newcode164
README.md:164: Clone the repo to you local machine. Then create a virtualenv and
install
For consistency it would be nice to leave an empty line after the header above
like in all other sections.

https://codereview.adblockplus.org/29849585/diff/29868573/README.md#newcode165
README.md:165: python abp there
Also consistency-related: we usually put a colon at the end of the line that
precedes a code block. If the colon doesn't fit, it probably should end in a
period, otherwise the line ending looks kind of bare.

[Tudor Avram, 2018-08-31 10:17:54]

Hi Vasily and Kirill,

I addressed the comments related to the README. 

Thanks,
Tudor.

https://codereview.adblockplus.org/29849585/diff/29868573/README.md
File README.md (right):

https://codereview.adblockplus.org/29849585/diff/29868573/README.md#newcode164
README.md:164: Clone the repo to you local machine. Then create a virtualenv and
install
On 2018/08/30 12:23:19, Vasily Kuznetsov wrote:
> For consistency it would be nice to leave an empty line after the header above
> like in all other sections.

Done.

https://codereview.adblockplus.org/29849585/diff/29868573/README.md#newcode165
README.md:165: python abp there
On 2018/08/30 12:23:19, Vasily Kuznetsov wrote:
> Also consistency-related: we usually put a colon at the end of the line that
> precedes a code block. If the colon doesn't fit, it probably should end in a
> period, otherwise the line ending looks kind of bare.

Done.

[Vasily Kuznetsov, 2018-08-31 12:35:41]

LGTM