Code review comment for lp:~michael.nelson/launchpad/627957-differences-schema-update

On Fri, Sep 10, 2010 at 2:48 PM, Abel Deuring
<email address hidden> wrote:
> Review: Approve code
> (13:23:56) adeuring: noodles775: _updateType() does not return anything. So, "return type_updated or status_updated" in update() looks a bit odd ;)
> (13:49:05) noodles775: adeuring: thanks, yeah, it should be returning whether it updated the model.
> (14:03:54) adeuring: noodles775: that's worth a test, I think

As it turned out, the private helper _updateType() does not need to
return anything, as the type will be updated if-and-only-if the
versions also update. I removed the expectation of a return value, and
renamed _updateStatus() to _updateVersionsAndStatus to reflect what it
is really doing.

> (14:04:33) noodles775: Indeed... I'd thought I had tested that, but it must have just been testing the return value of the other _updated helper method.
> (14:10:40) adeuring: noodles775: the IPropertyCacheManager(ds_diff).clear() in a test look a bit odd: If this is necessary in a test, wouldn't it be necessary in "real life"?
> (14:16:38) noodles775: adeuring: It's necessary there because the test creates a new difference and then publishes a new version of the package, before then updating the difference.
> (14:16:54) noodles775: so no, it's not something that would happen in the duration of a request.
> (14:17:57) adeuring: noodles775: could you add comments with this explanation to the cache.clear() calls?
> (14:18:30) noodles775: adeuring: to each one? (I think there are 8... how about at the top of the test case?)
> (14:18:49) adeuring: noodles775: yes, that should be enough ;)

{{{
14:51 < noodles775> Thanks adeuring. I was also thinking, it may makes
sense to clear the cache at the start of the update() method too,
which would remove the need for them in the tests.
14:51 < adeuring> noodles775: yes, that would be even better
}}}

So I did this, and added a comment as to why it is cleared there even
though there is currently no need. I also cleared the cache before
returning the factory object so that the factory returns an object as
if it had been freshly obtained from the store.

Incremental here: http://pastebin.ubuntu.com/491627/

Thanks!