Issue 5456 - Add support to specify urlFilters in the notification repository

Issue 5456 - Add support to specify urlFilters in the notification repository

[Vasily Kuznetsov, 2017-08-03 17:08:52]

Hi Winsley,

This also looks right, except for some coding style nitpicking (see below). Let
me know, if any of that seems arbitrary or unclear, it might be good to add it
to the style guide and/or flake8-eyeo.

Cheers,
Vasily

https://codereview.adblockplus.org/29500676/diff/29500677/sitescripts/notific...
File sitescripts/notifications/parser.py (right):

https://codereview.adblockplus.org/29500676/diff/29500677/sitescripts/notific...
sitescripts/notifications/parser.py:96: current['urlFilters'] =
["{}^$document".format(v.upper())
Our style guide recommends single quotes and using "+" for concatenating just
two strings. I believe flake8-eyeo should complain about those. Also the
indentation of the second line should align with the opening brace in this case
(not sure if flake8 would complain, but we've basically agreed to use one of the
two styles:

    foo = [bar
               for bar in baz]
    foo = [
        bar
        for bar in baz
    ]

when multiline literals, comprehension expressions or long parameter lists need
to be split over several lines). So probably something like this would be more
"kosher":

            current['urlFilters'] = [v.upper() + '$document'
	    			     for v in value.split()]

[wspee, 2017-08-04 08:41:23]

https://codereview.adblockplus.org/29500676/diff/29500677/sitescripts/notific...
File sitescripts/notifications/parser.py (right):

https://codereview.adblockplus.org/29500676/diff/29500677/sitescripts/notific...
sitescripts/notifications/parser.py:96: current['urlFilters'] =
["{}^$document".format(v.upper())
On 2017/08/03 17:08:52, Vasily Kuznetsov wrote:
> Our style guide recommends single quotes and using "+" for concatenating just
> two strings. I believe flake8-eyeo should complain about those. Also the
> indentation of the second line should align with the opening brace in this
case
> (not sure if flake8 would complain, but we've basically agreed to use one of
the
> two styles:
> 
>     foo = [bar
>                for bar in baz]
>     foo = [
>         bar
>         for bar in baz
>     ]
> 
> when multiline literals, comprehension expressions or long parameter lists
need
> to be split over several lines). So probably something like this would be more
> "kosher":
> 
>             current['urlFilters'] = [v.upper() + '$document'
> 	    			     for v in value.split()]

Done.

[Vasily Kuznetsov, 2017-08-04 14:23:04]

Just realized now that my first example above is misindented. Should be:

    foo = [bar
           for bar in baz]

Sorry about that. And LGTM.

[wspee, 2017-09-21 08:15:07]

On 2017/08/04 14:23:04, Vasily Kuznetsov wrote:
> Just realized now that my first example above is misindented. Should be:
> 
>     foo = [bar
>            for bar in baz]
> 
> Sorry about that. And LGTM.

I also added tests for urlFilters

[Vasily Kuznetsov, 2017-09-21 09:44:10]

https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notific...
File sitescripts/notifications/test/parser.py (right):

https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notific...
sitescripts/notifications/test/parser.py:166: self.assertEqual(
These following two asserts would look even better with `assert` instead of
`self.assertEqual`.

[wspee, 2017-09-21 10:11:34]

https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notific...
File sitescripts/notifications/test/parser.py (right):

https://codereview.adblockplus.org/29500676/diff/29551582/sitescripts/notific...
sitescripts/notifications/test/parser.py:166: self.assertEqual(
On 2017/09/21 09:44:10, Vasily Kuznetsov wrote:
> These following two asserts would look even better with `assert` instead of
> `self.assertEqual`.

Done.

[Vasily Kuznetsov, 2017-09-21 11:11:01]

LGTM