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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
