Code review comment for lp:~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal

207 + @cachedproperty
208 + def preview_diffs(self):
209 + return Store.of(self).find(
210 + PreviewDiff, PreviewDiff.merge_proposal_id == self.id).order_by(
211 + PreviewDiff.date_created)

Caching a ResultSet achieves nothing. You probably want to cache a listified version instead.

213 + @property
214 + def preview_diff(self):
215 + diffs = list(self.preview_diffs)
216 + if diffs:
217 + return diffs[-1]
218 + return None

Materialising all of the PreviewDiffs just for this is probably pointless, particularly given how cheap it is to find just the latest. I'd probably move the ResultSet into _preview_diffs, then have preview_diffs listify and cache it, and preview_diff grab the first and cache it. If you wanted to be excessively performant you could even have preview_diff check is preview_diffs was cached, and if so just grab the first from there, but that's probably pointless.

292 + for previewdiff in preview_diffs:
293 + cache = get_property_cache(previewdiff.merge_proposal)
294 + cache.preview_diffs.append(previewdiff)

Is preloading preview_diffs valuable here, or would it be sufficient to simply preload the latest, preview_diff?

371 + @property
372 def branch_merge_proposal(self):
373 - # This can turn into return self.merge_proposal when it's populated.
374 - if self.merge_proposal:
375 - return self.merge_proposal
376 - return self._branch_merge_proposal
377 + return self.merge_proposal

Why does this still exist?