Code review comment for lp:~edwin-grubbs/launchpad/bug-490659-series-timeout

Revision history for this message
Robert Collins (lifeless) wrote :

69- def substitude_filled_numbers(match):
70+
71+ def substitute_filled_numbers(match):

That new VWS breaks up a small function - closures like that are more readable when they look like one conceptual thing rather than two IMO; please consider removing the new VWS.

I think the index is new? Needs a new stamp from stuart if so.

Lastly, it seems to me that LBYL isn't needed here: surely doing *neither* a .count() nor a .any() is appropriate: rather just iterate the latest_milestones, and if the iterator outputs no rows don't show the table? Perhaps we don't have a construct for doing that; if thats the case I'm happy with this approach, but suggest that you file a bug saying we should have such a construct - it will be the least work of all and thus fastest.

review: Approve

« Back to merge proposal