Issue 6582297721569280: Issue 2275 - Parse notification groups

Issue 6582297721569280: Issue 2275 - Parse notification groups (Closed)

Created:
April 11, 2015, 9:13 p.m. by Felix Dahlke

Modified:
May 22, 2015, 1:05 p.m.

Reviewers:
Sebastian Noack

CC:
Wladimir Palant

Visibility:
Public.

Patch Set 2 : Address comments, parse sample #

Patch Set 3 : Rebased #

Created: April 15, 2015, 9:26 a.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+26 lines, -12 lines)			Patch
	M	sitescripts/management/bin/generateNotifications.py	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
	M	sitescripts/notifications/parser.py	View	1 2	1 chunk	+23 lines, -12 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

Sebastian Noack

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/sitescripts/management/bin/generateNotifications.py File sitescripts/management/bin/generateNotifications.py (right): http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/sitescripts/management/bin/generateNotifications.py#newcode29 sitescripts/management/bin/generateNotifications.py:29: filter(lambda notification: not "variants" in notification, 1. list comprehension ...

April 13, 2015, 9:04 a.m. (2015-04-13 09:04:36 UTC) #2

Felix Dahlke

New patch set is up. http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/sitescripts/management/bin/generateNotifications.py File sitescripts/management/bin/generateNotifications.py (right): http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/sitescripts/management/bin/generateNotifications.py#newcode29 sitescripts/management/bin/generateNotifications.py:29: filter(lambda notification: not "variants" ...

April 15, 2015, 8:53 a.m. (2015-04-15 08:53:43 UTC) #3

New patch set is up.

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/site...
File sitescripts/management/bin/generateNotifications.py (right):

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/site...
sitescripts/management/bin/generateNotifications.py:29: filter(lambda
notification: not "variants" in notification,
On 2015/04/13 09:04:37, Sebastian Noack wrote:
> 1. list comprehension > callback
> 2. |x not in y| > |not x in y|
> 
> [x for x in notifications["notifications"] if "variants" not in x]

Done.

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/site...
File sitescripts/notifications/parser.py (right):

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/site...
sitescripts/notifications/parser.py:62: notification["variants"].append(current)
On 2015/04/13 09:04:37, Sebastian Noack wrote:
> Those 3 lines can be expressed in a single line:
> 
> notification.setdefault("variants", []).append(current)

Done.

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/site...
sitescripts/notifications/parser.py:96: for text_key in ["title", "message"]:
On 2015/04/13 09:04:37, Sebastian Noack wrote:
> Please use tuples for sequences that don't need to be mutable.

Done.

http://codereview.adblockplus.org/6582297721569280/diff/5144752345317376/site...
sitescripts/notifications/parser.py:97: if "en-US" not in notification[text_key]
and \
On 2015/04/13 09:04:37, Sebastian Noack wrote:
> It seems this this logic can be simplified:
> 
>   for text_key in ("title", "message"):
>     has_default_locale = lambda variant: "en-US" in variant[text_key]
>     if has_default_locale(notification):
>       continue
>     if all(map(has_default_locale, notification.get("variants", []))):
>       continue

Indeed, ended up with something similar to this.

Note that I went for a def statement for has_default_locale, PEP-8 says: "Always
use a def statement instead of an assignment statement that binds a lambda
expression directly to an identifier. "

LGTM

Expand All Messages | Collapse All Messages