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

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

Brad also pointed that out, and it has been fixed.

> 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 actually disagree that it makes it less readable. Secondly, pocketlint complains when you don't have a blank line above a function definition.

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

The index isn't new. Since I included a full diff after the first set of changes instead of an incremental diff, it may have appeared new, but it was in the patch file below the db function before the db function was moved to trusted.sql.

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

The problem with not checking .any() before iterating is that a storm ResultSet object will query the database every time you iterate over it; therefore, you would have to create a new cached property on the view that would hold a list in order to eliminate the query. We can't have the model just cache the property as a list for us, since the whole point of this refactoring was to allow the view code to slice the model's attribute without the model first querying a really large number of rows.

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

I definitely think there is a better universal solution to this, but it will need more discussion on the mailing list, since either template/view coding practices or storm's ResultSet will have to change significantly. Also, to put it into perspective, we are talking about eliminating one or two single-row queries, compared to the 700-rows of results that this branch eliminates, so improving this might not be as urgent as other optimizations.