launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

Review: Approve

Jeroen--

Looks good. One suggestion:

Instead of 

13      + if template_id not in cached_potmsgsets:
14	+ cached_potmsgsets[template_id] = get_potmsgset_ids(template_id)
15	+ potmsgset_ids = cached_potmsgsets[template_id]

I think you can just do 

13      + potmsgset_ids = cached_potmsgsets.setdefault(template_id, get_potmsgset_ids(template_id))

dict.setdefault returns the value if the key exists, otherwise it function like dict.get(), returning the second parameter, but *also* setting it for the supplied key.

It's really just an LoC reduction, so not a major improvement, but it's a bit cleaner than the if clause set up. I'll leave it to you whether to make that change or not. I'll leave it to you whether or not you want to implement the change.

It doesn't look like this total arc of work reduces LoC, but I'm not familiar enough with everything you've been doing in the LP code base lately to know if you've got credit or not. I don't want to block a fix for a critical, so I'll just leave this as a reminder to please try and reduce the overall LoC going forward if you haven't already.

Thanks!
-- 
https://code.launchpad.net/~jtv/launchpad/bug-994650-cache-potmsgsets/+merge/105193
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.