Issue 7471 - Add an API for working with blocks of filters

Issue 7471 - Add an API for working with blocks of filters

Repository: https://hg.adblockplus.org/python-abp
Base revision: 1e2e67e2d92b

[Vasily Kuznetsov, 2019-05-09 11:19:00]

Hi Tudor,

This is the new python-abp feature I have mentioned earlier today. Let me know
if you have any questions about how it's supposed to work or about the code.

Cheers,
Vasily

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/parser.py
File abp/filters/parser.py (right):

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/parser....
abp/filters/parser.py:110: def _option_list_to_dict(options):
These two functions were moved from `rpy.py` to provide `.to_dict()` directly on
the parsed lines. This is useful for `.to_dict()` of the filters blocks.

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

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/rpy.py#...
abp/filters/rpy.py:73: return strings2utf8(parse_line(text, mode).to_dict())
This is now a method of filter lines.

https://codereview.adblockplus.org/30053555/diff/30054555/tests/test_rpy.py
File tests/test_rpy.py (left):

https://codereview.adblockplus.org/30053555/diff/30054555/tests/test_rpy.py#o...
tests/test_rpy.py:138: @pytest.mark.parametrize('foo,bar', [
The code that's tested here is now in `parser.py` and the test is not really
needed since it's well covered by the following two tests and the tests of
filters blocks.

[Tudor Avram, 2019-05-09 15:51:04]

Hi Vasily, Mundo!

Looks pretty good, only had 3 comments. 

@Mundo: Can you please take a look at the comment regarding the dict structure
of the blocks and let us know what you think? 

Thanks,
Tudor.

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks.py
File abp/filters/blocks.py (right):

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:73: if name.startswith('_') or name in {'filters',
'description'}:
If we're doing the validation, I guess it might make sense to add `to_dict` to
the invalid variable names. What do you think?

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:85: setattr(self, name, value)
Any reason why you decided on adding the variables straight up as keys of the
dict result? Why are you not storing it as a separate inner-dict?  Something
like this:
```
{
    'description': 'foo'
    'variables': {
        'bar': 'baz'
    }
    'filters': {
        '@@some-filter'
    }
}
```
This would make it easier to process afterwards IMO and will also remove the
validation step.

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:114: blocks : iterable of FiltersBlock.
Nit: It looks like you're returning a generator. Technically speaking, I know
generators are iterables, but maybe we can be more specific. What do you think?

[Vasily Kuznetsov, 2019-05-09 15:59:43]

Hi Tudor!

Thanks for the review. I've answered inline. Let's wait for Edmundo's reply and
then I'll fix it.

Cheers,
Vasily

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks.py
File abp/filters/blocks.py (right):

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:73: if name.startswith('_') or name in {'filters',
'description'}:
On 2019/05/09 15:51:03, Tudor Avram wrote:
> If we're doing the validation, I guess it might make sense to add `to_dict` to
> the invalid variable names. What do you think?

Good catch, thanks. This was implemented before .to_dict() and then I forgot to
add it.

Let's see what Edmundo says about the inner dict proposal and if not, I'll add
it.

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:85: setattr(self, name, value)
On 2019/05/09 15:51:03, Tudor Avram wrote:
> Any reason why you decided on adding the variables straight up as keys of the
> dict result? Why are you not storing it as a separate inner-dict?  Something
> like this:
> ```
> {
>     'description': 'foo'
>     'variables': {
>         'bar': 'baz'
>     }
>     'filters': {
>         '@@some-filter'
>     }
> }
> ```
> This would make it easier to process afterwards IMO and will also remove the
> validation step.

Edmundo: I think I agree with Tudor here, what do you think? Would having
variables in an inner dictionary be convenient for you?

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:114: blocks : iterable of FiltersBlock.
On 2019/05/09 15:51:03, Tudor Avram wrote:
> Nit: It looks like you're returning a generator. Technically speaking, I know
> generators are iterables, but maybe we can be more specific. What do you
think?

This is true but I didn't want to constrain the implementation to returning a
generator specifically. Why? Well... mostly it doesn't matter for the user of
the API so why promise more than you need. I don't feel too strong about this
though.

[Vasily Kuznetsov, 2019-05-09 16:07:31]

Edmundo has told me via IRC that nested variables approach is fine for him. I
will adjust the code and remove the validation.

[Vasily Kuznetsov, 2019-05-09 16:24:18]

Hi Tudor,

I have implemented your suggestion. How does it look?

Cheers,
Vasily

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks.py
File abp/filters/blocks.py (right):

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:73: if name.startswith('_') or name in {'filters',
'description'}:
On 2019/05/09 15:59:42, Vasily Kuznetsov wrote:
> On 2019/05/09 15:51:03, Tudor Avram wrote:
> > If we're doing the validation, I guess it might make sense to add `to_dict`
to
> > the invalid variable names. What do you think?
> 
> Good catch, thanks. This was implemented before .to_dict() and then I forgot
to
> add it.
> 
> Let's see what Edmundo says about the inner dict proposal and if not, I'll add
> it.

Now this validation is no longer needed.

https://codereview.adblockplus.org/30053555/diff/30054555/abp/filters/blocks....
abp/filters/blocks.py:85: setattr(self, name, value)
On 2019/05/09 15:59:42, Vasily Kuznetsov wrote:
> On 2019/05/09 15:51:03, Tudor Avram wrote:
> > Any reason why you decided on adding the variables straight up as keys of
the
> > dict result? Why are you not storing it as a separate inner-dict?  Something
> > like this:
> > ```
> > {
> >     'description': 'foo'
> >     'variables': {
> >         'bar': 'baz'
> >     }
> >     'filters': {
> >         '@@some-filter'
> >     }
> > }
> > ```
> > This would make it easier to process afterwards IMO and will also remove the
> > validation step.
> 
> Edmundo: I think I agree with Tudor here, what do you think? Would having
> variables in an inner dictionary be convenient for you?

Done.

[Tudor Avram, 2019-05-09 17:06:01]

LGTM