Issue 29570562: Issue 5827 - Assign new groups even if prior groups are present (Closed)
DescriptionIssue 5827 - Assign new groups even if prior groups are present
Patch Set 1 #
Total comments: 11
Patch Set 2 : Revert back to mocking load_notification #
Total comments: 3
Patch Set 3 : Removed return to make in-place editing more apparent #
MessagesTotal messages: 8
https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/test/notification.py (left): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:136: def test_notification_variant_merged(self): This "merging" is not possible with the notification parser so I had to remove this test. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:315: {'id': '2', 'title': {'en-US': ''}, 'message': {}}, This is not possible with the parser, so I removed the notification from the new test. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/test/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:35: self.repo_dir = tempfile.mkdtemp('md_repo') As discussed: This created a new repository for every test. This make sense in terms of test isolation but it means that this is quite slow (whole suit ~8s). I explored different options to make it faster (hglib, copy repo instead of creating a new one) but the main performance decrease comes from making commits so I have abandoned the idea. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/web/notification.py:115: groups = _assign_groups(groups, notifications) Ideally I would like to combine _determine_groups and _assign_groups because the two steps are not really independent. But then I would be changing a lot of code already and then I would like to change some more and so on ... based on apple pricing logic I would end up with a complete rewrite which is probably a bad idea because of [0]. So it's Darwin for now I fear :) [0] https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/
Hi Winsley, Sorry if you spent much time converting the tests in `test/notifications.py` to use a real Mercurial repo based on our conversation. Now, after looking at the whole test suite, this approach doesn't seem to be the right one (see comments below). Perhaps we should have looked at the code together instead of having an abstract conversation :D Let me know if you think we were right though! Cheers, Vasily https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/test/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:35: self.repo_dir = tempfile.mkdtemp('md_repo') On 2017/10/09 09:12:03, wspee wrote: > As discussed: This created a new repository for every test. This make sense in > terms of test isolation but it means that this is quite slow (whole suit ~8s). I > explored different options to make it faster (hglib, copy repo instead of > creating a new one) but the main performance decrease comes from making commits > so I have abandoned the idea. Hm. Now after I looked at the whole test suite it seems like mocking `load_notifications` out in this file is the right approach, since we have a whole separate set of tests for it in `parser.py` (and those tests actually use a pretty cool way to quickly simulate `hg`, which I will neither endorse nor disavow). So if there's something that `load_notifications` fails to do, we should add a test for that to `parser.py`, not to here. Does this make sense? https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:130: self.add_notifications([ There is and was a lot of code duplication in this file. If you end up touching these methods, it would probably be good to move those repeated data structures out as constants or reuse them in some other way. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/web/notification.py:43: if notification['id'] in [g['id'] for g in groups]: This should be a set and probably should be pre-calculated. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/web/notification.py:115: groups = _assign_groups(groups, notifications) On 2017/10/09 09:12:03, wspee wrote: > Ideally I would like to combine _determine_groups and _assign_groups because the > two steps are not really independent. > > But then I would be changing a lot of code already and then I would like to > change some more and so on ... based on apple pricing logic I would end up with > a complete rewrite which is probably a bad idea because of [0]. > > So it's Darwin for now I fear :) > > [0] https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/ Since we remove inactive notifications in the middle, notifications parameter in the two calls is not the same. Seems like you'd have to move this filtering inside to combine two functions, which would probably make them do too many unrelated things. I would say the current separation makes some sense.
https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/test/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:35: self.repo_dir = tempfile.mkdtemp('md_repo') On 2017/10/09 22:29:11, Vasily Kuznetsov wrote: > On 2017/10/09 09:12:03, wspee wrote: > > As discussed: This created a new repository for every test. This make sense in > > terms of test isolation but it means that this is quite slow (whole suit ~8s). > I > > explored different options to make it faster (hglib, copy repo instead of > > creating a new one) but the main performance decrease comes from making > commits > > so I have abandoned the idea. > > Hm. Now after I looked at the whole test suite it seems like mocking > `load_notifications` out in this file is the right approach, since we have a > whole separate set of tests for it in `parser.py` (and those tests actually use > a pretty cool way to quickly simulate `hg`, which I will neither endorse nor > disavow). So if there's something that `load_notifications` fails to do, we > should add a test for that to `parser.py`, not to here. Does this make sense? No worries. Half of it was based on my assumptions anyways :). I see your point. Done. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/test/notification.py:130: self.add_notifications([ On 2017/10/09 22:29:11, Vasily Kuznetsov wrote: > There is and was a lot of code duplication in this file. If you end up touching > these methods, it would probably be good to move those repeated data structures > out as constants or reuse them in some other way. Acknowledged. https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29570563/sitescripts/notific... sitescripts/notifications/web/notification.py:115: groups = _assign_groups(groups, notifications) On 2017/10/09 22:29:11, Vasily Kuznetsov wrote: > On 2017/10/09 09:12:03, wspee wrote: > > Ideally I would like to combine _determine_groups and _assign_groups because > the > > two steps are not really independent. > > > > But then I would be changing a lot of code already and then I would like to > > change some more and so on ... based on apple pricing logic I would end up > with > > a complete rewrite which is probably a bad idea because of [0]. > > > > So it's Darwin for now I fear :) > > > > [0] > https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/ > > Since we remove inactive notifications in the middle, notifications parameter in > the two calls is not the same. Seems like you'd have to move this filtering > inside to combine two functions, which would probably make them do too many > unrelated things. I would say the current separation makes some sense. Acknowledged.
LGTM
LGTM with a nit. https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notific... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notific... sitescripts/notifications/web/notification.py:55: return groups Nit: The way this function works now, returning groups is redundant. It just assigns the groups directly.
https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notific... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notific... sitescripts/notifications/web/notification.py:55: return groups On 2017/10/10 14:10:01, Felix Dahlke wrote: > Nit: The way this function works now, returning groups is redundant. It just > assigns the groups directly. I agree. Also, if we make it return None, the fact that `groups` is modified in place will be more obvious from the signature.
https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notific... File sitescripts/notifications/web/notification.py (right): https://codereview.adblockplus.org/29570562/diff/29572579/sitescripts/notific... sitescripts/notifications/web/notification.py:55: return groups On 2017/10/10 14:17:18, Vasily Kuznetsov wrote: > On 2017/10/10 14:10:01, Felix Dahlke wrote: > > Nit: The way this function works now, returning groups is redundant. It just > > assigns the groups directly. > > I agree. Also, if we make it return None, the fact that `groups` is modified in > place will be more obvious from the signature. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
