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

Revision history for this message
Abel Deuring (adeuring) wrote :

(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
(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 ;)

review: Approve (code)

« Back to merge proposal