launchpad-dev team mailing list archive

[Andrew Bennetts <andrew.bennetts@xxxxxxxxxxxxx>]

Julian Edwards wrote:
[...]
> At the very least, if the reviewer did understand the branch, I'd expect a 
> comment to that effect confirming the action taken in the changes.

I agree.  I think it's probably a good idea for reviews to have a “cover
letter” of sorts too.

Just as a merge proposal has an overall description plus the diff of
individual lines changed, a review benefits from an overall summary in
addition to the line-by-line comments.  If I write a review that
comments only on the bits that I think should change, even though I
liked the overall patch, then I feel I'm not really communicating what I
thought to the submitter.  Yes, I thought those bits should change, but
overall I (usually) quite liked it.

So I try to say at least “This looks good to me.  Just some minor nits:”
or similar.  I think it often can be that simple, although for harder to
understand branches of the sort Julian is referring to I hope I go
slightly further!

-Andrew.