launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Thanks for the review!  I was wondering when the LoC thing would come up. :)  In defense of Francis' note, not entirely without self-interest, I'm also obviating considerable command-lining and human-scripting (“disable X, click Y”) for every Ubuntu cycle.


> 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))

That would evaluate get_potmsgset_ids(template_id) before the setdefault() call.  There's not a very great distance between the call constructing a query that will be executed only when needed, and it also executing the query in order to return some other data structure.  So it would make the caching a bit brittle!

Does make me wonder if dict.setdefault shouldn't accept a callable plus arguments so that it can evaluate lazily.


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