Issue 2707 - Remove start/end from notifications

Issue 2707 - Remove start/end from notifications

[Felix Dahlke, 2015-07-29 10:45:11]

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

https://codereview.adblockplus.org/29322888/diff/29322889/sitescripts/notific...
sitescripts/notifications/parser.py:125: if "start" in notification:
Not entirely sure on how to best change this. This makes sense logically, but I
wonder if it'd be more readable if we just remove "start" and "end" from all
notifications before `notifications.append(notification)` below. What do you
think?

[Sebastian Noack, 2015-07-29 11:28:54]

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

https://codereview.adblockplus.org/29322888/diff/29322889/sitescripts/notific...
sitescripts/notifications/parser.py:125: if "start" in notification:
On 2015/07/29 10:45:11, Felix Dahlke wrote:
> Not entirely sure on how to best change this. This makes sense logically, but
I
> wonder if it'd be more readable if we just remove "start" and "end" from all
> notifications before `notifications.append(notification)` below. What do you
> think?

This wouldn't be the same. With this patch you only remove "start" and "end" if
they don't validate against the current time. If you remove them unconditionally
below you would always remove them.

If the latter is the intended behavior I'd go with following code:

  start = notifications.pop("start", None) 
  if start is not None and current_time < start:
    continue

[Felix Dahlke, 2015-07-29 14:30:18]

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

https://codereview.adblockplus.org/29322888/diff/29322889/sitescripts/notific...
sitescripts/notifications/parser.py:125: if "start" in notification:
On 2015/07/29 11:28:54, Sebastian Noack wrote:
> On 2015/07/29 10:45:11, Felix Dahlke wrote:
> > Not entirely sure on how to best change this. This makes sense logically,
but
> I
> > wonder if it'd be more readable if we just remove "start" and "end" from all
> > notifications before `notifications.append(notification)` below. What do you
> > think?
> 
> This wouldn't be the same. With this patch you only remove "start" and "end"
if
> they don't validate against the current time. If you remove them
unconditionally
> below you would always remove them.
> 
> If the latter is the intended behavior I'd go with following code:
> 
>   start = notifications.pop("start", None) 
>   if start is not None and current_time < start:
>     continue

Yeah it's fine to always remove them. I'll do it the way you suggested, seems to
be the most readable alternative.

[Sebastian Noack, 2015-07-29 14:32:51]

LGTM