Issue 29527607: Issue 5458, 5457 - Refactor _parse_targetspec, add support for locales and blockTotal (Closed)
DescriptionIssue 5458, 5457 - Refactor _parse_targetspec, add support for locales and blockTotal
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added test for target parsing #
Total comments: 3
Patch Set 3 : Use assert instead of assertEqual, updated README.md #
Total comments: 5
Patch Set 4 : Use assert instead of assertEqual across the board, also test no additional targets are present #
MessagesTotal messages: 12
The notification repository syntax changed, see: https://issues.adblockplus.org/ticket/5457#comment:5 https://issues.adblockplus.org/ticket/5458#comment:5 So the old reviews are irrelevant now (100% or the src has changed). Sorry about that, I should have submitted them only after the changes have landed. The changes have landed now so it's safe now: https://issues.adblockplus.org/ticket/5459#comment:12 https://issues.adblockplus.org/ticket/5460#comment:7
Hi Winsley, The code looks good to me but perhaps it would make sense for Wladimir to review it as too since he was implementing related changes in the extension. It would also be good to add the tests for new functionality and even gooder if the syntax of the source files and of resulting JSON was documented in the README, but the last part might be a bit offtopic for this commit. Cheers, Vasily https://codereview.adblockplus.org/29527607/diff/29527608/sitescripts/notific... File sitescripts/notifications/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29527608/sitescripts/notific... sitescripts/notifications/parser.py:40: (r'^(blockedTotal)(=|\>=|\<=)(\d+)$', { It seems like we're adding new variables and logic here, do you think it would make sense to also add tests for this?
On 2017/08/25 18:03:06, Vasily Kuznetsov wrote: > Hi Winsley, > > The code looks good to me but perhaps it would make sense for Wladimir to review > it as too since he was implementing related changes in the extension. > > It would also be good to add the tests for new functionality and even gooder if > the syntax of the source files and of resulting JSON was documented in the > README, but the last part might be a bit offtopic for this commit. > > Cheers, > Vasily > > https://codereview.adblockplus.org/29527607/diff/29527608/sitescripts/notific... > File sitescripts/notifications/parser.py (right): > > https://codereview.adblockplus.org/29527607/diff/29527608/sitescripts/notific... > sitescripts/notifications/parser.py:40: (r'^(blockedTotal)(=|\>=|\<=)(\d+)$', { > It seems like we're adding new variables and logic here, do you think it would > make sense to also add tests for this? It took me a while but I added not only the good but also the gooder ;). I added Wladimir as a reviewer so he can look over the changes. I added tests for the target parameter. I also wrote specifications/documentations for the notification repository format, see https://bitbucket.org/adblockplus/spec/pull-requests/59. I'll add it to sitescripts/notifications/README.md once it has been merged.
Hi Winsley, It looks good, but you could make it even nicer by replacing `self.assertEqual` with a simple `assert` (see below). Cheers, Vasily https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... sitescripts/notifications/test/parser.py:192: self.assertEqual(len(notifications), 17) These tests are run with pytest nowadays, so you can use assert len(notifications) == 17 instead of `self.assertEqual`. It's more compact and readable and pytest produces very helpful failure messages when the assertion fails.
On 2017/09/20 15:45:33, Vasily Kuznetsov wrote: > Hi Winsley, > > It looks good, but you could make it even nicer by replacing `self.assertEqual` > with a simple `assert` (see below). > > Cheers, > Vasily > > https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... > File sitescripts/notifications/test/parser.py (right): > > https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... > sitescripts/notifications/test/parser.py:192: > self.assertEqual(len(notifications), 17) > These tests are run with pytest nowadays, so you can use > > assert len(notifications) == 17 > > instead of `self.assertEqual`. It's more compact and readable and pytest > produces very helpful failure messages when the assertion fails. The notification spec has been merged so I also added a link to it from the notifications README.md
https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... sitescripts/notifications/test/parser.py:192: self.assertEqual(len(notifications), 17) On 2017/09/20 15:45:33, Vasily Kuznetsov wrote: > These tests are run with pytest nowadays, so you can use > > assert len(notifications) == 17 > > instead of `self.assertEqual`. It's more compact and readable and pytest > produces very helpful failure messages when the assertion fails. Done.
https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... sitescripts/notifications/test/parser.py:193: assert len(notifications) == 17 I'd recommend switching back to self.assertEqual() here. Both will throw AssertionError but currently it won't have any information beyond the stack trace. Calling self.assertEqual() on the other hand will produce a message like "AssertionError: 16 != 17" which is more meaningful. Yes, I tried this. https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... sitescripts/notifications/test/parser.py:196: notifications[0]['targets'][0]['extension'], 'adblockplus') You should verify that there are no additional targets either. This should work: self.assertEqual(notifications[0]['targets'], [{'extension': 'adblockplus'}])
https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... sitescripts/notifications/test/parser.py:192: self.assertEqual(len(notifications), 17) On 2017/09/20 15:45:33, Vasily Kuznetsov wrote: > These tests are run with pytest nowadays, so you can use > > assert len(notifications) == 17 > > instead of `self.assertEqual`. It's more compact and readable and pytest > produces very helpful failure messages when the assertion fails. I actually meant it for all new assertions (the ones below would benefit more than this one in terms of readability). https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... sitescripts/notifications/test/parser.py:193: assert len(notifications) == 17 On 2017/09/21 09:24:02, Wladimir Palant wrote: > I'd recommend switching back to self.assertEqual() here. Both will throw > AssertionError but currently it won't have any information beyond the stack > trace. Calling self.assertEqual() on the other hand will produce a message like > "AssertionError: 16 != 17" which is more meaningful. Yes, I tried this. Here's the error message that I get (after changing 17 to 16): > assert len(notifications) == 16 E AssertionError: assert 17 == 16 E + where 17 = len([{'id': '1', 'message': {}, 'severity': 'information', 'targets': [{'extension': 'adblockplus'}], ...}, {'id': '2', 'm...severity': 'information', 'targets': [{'applicationMaxVersion': '1.2.3', 'applicationMinVersion': '1.2.3'}], ...}, ...]) Seems detailed and clear. Given that these tests run under pytest and tox (see https://hg.adblockplus.org/sitescripts/file/tip/README.md) we might as well use the benefits of those tools.
On 2017/09/21 09:41:30, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... > File sitescripts/notifications/test/parser.py (right): > > https://codereview.adblockplus.org/29527607/diff/29541700/sitescripts/notific... > sitescripts/notifications/test/parser.py:192: > self.assertEqual(len(notifications), 17) > On 2017/09/20 15:45:33, Vasily Kuznetsov wrote: > > These tests are run with pytest nowadays, so you can use > > > > assert len(notifications) == 17 > > > > instead of `self.assertEqual`. It's more compact and readable and pytest > > produces very helpful failure messages when the assertion fails. > > I actually meant it for all new assertions (the ones below would benefit more > than this one in terms of readability). Done.
https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... File sitescripts/notifications/test/parser.py (right): https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... sitescripts/notifications/test/parser.py:193: assert len(notifications) == 17 On 2017/09/21 09:41:30, Vasily Kuznetsov wrote: > On 2017/09/21 09:24:02, Wladimir Palant wrote: > > I'd recommend switching back to self.assertEqual() here. Both will throw > > AssertionError but currently it won't have any information beyond the stack > > trace. Calling self.assertEqual() on the other hand will produce a message > like > > "AssertionError: 16 != 17" which is more meaningful. Yes, I tried this. > > Here's the error message that I get (after changing 17 to 16): > > > assert len(notifications) == 16 > E AssertionError: assert 17 == 16 > E + where 17 = len([{'id': '1', 'message': {}, 'severity': > 'information', 'targets': [{'extension': 'adblockplus'}], ...}, {'id': '2', > 'm...severity': 'information', 'targets': [{'applicationMaxVersion': '1.2.3', > 'applicationMinVersion': '1.2.3'}], ...}, ...]) > > Seems detailed and clear. Given that these tests run under pytest and tox (see > https://hg.adblockplus.org/sitescripts/file/tip/README.md) we might as well use > the benefits of those tools. I agree with Vasily, using assert only provides additional information IMHO. https://codereview.adblockplus.org/29527607/diff/29551575/sitescripts/notific... sitescripts/notifications/test/parser.py:196: notifications[0]['targets'][0]['extension'], 'adblockplus') On 2017/09/21 09:24:02, Wladimir Palant wrote: > You should verify that there are no additional targets either. This should work: > > self.assertEqual(notifications[0]['targets'], [{'extension': 'adblockplus'}]) Done.
LGTM
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
