launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

83	+ store = IStore(Bug)
84	+ store.flush()

The set() will implicitly flush before it does anything. Explicit flushes are largely obsolete in the post-SQLObject world.

90	+ store.flush()

Likewise here. Additionally, I'm not sure if the set() will have implicitly invalidated any heat/heat_last_updated values that we might have cached already, so subsequent reads of the fields in this transaction may end up out of date. We might not care, but a comment might be a good idea.

168	+ self._flushAndInvalidate()

Same here. It's possibly reasonable to invalidate just self, but it would be nicer to invalidate the relevant columns with AutoReload, ideally across all affected objects in the cache.

193	duplicate.addChange(
194	- change, empty_recipients, deferred=True)
195	+ change, empty_recipients, deferred=True,
196	+ update_heat=False)
197	+ affected_bug_ids.add(duplicate.id)

We don't technically need to update heat here, since the only change we've made is switching duplicateof from one bug to another. But it's preserving existing cheap behaviour, so I guess it makes sense to keep it.

287	+def update(where, col_values, values=None):

Does this interface provide a significant usability/readability benefit over the direct Storm IStore(cls).find(cls, where).set(foo=bar) version? I'm not sure if it does.

-- 
https://code.launchpad.net/~wallyworld/launchpad/markUserAffected-queries-1066647/+merge/129793
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.