On Thursday 21 October 2010 11:32:29 Graham Binns wrote: > On 21 October 2010 11:20, Julian Edwards <[email protected]> wrote: > > What I find extremely irritating is nitpicking over minor formatting and > > grammatical issues. The first thing that pops into my head when someone > > does this, with no other comments about my code, is "you've not really > > looked at what this patch is doing, have you?" > > I hope that the second thing you think is "that's unfair of me, bad > Julian."
Not really. It does largely depend on the style of review but I've had reviews of fairly complicated changes before where I was expecting questions about how something worked (this is Soyuz, right?) and had nothing except a request to add a full stop at the end of a comment (for example). 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. This could also be a symptom of reviewer fatigue though. > Remember that how you look at the code style really depends upon how > you treat reviews. I was told when I started out that they were about > maintaining code consistency as much as making sure that the patch > worked. That has changed over time but - and this is the real > annoyance here, I suspect - it's not consistent from reviewer to > reviewer. Also, how many times have you read a document in English and > been distracted by errors in spelling, punctuation and grammar? For > some reviewers (for me, at least), errors in the coding style are just > as distracting. Of course, because we do our job right (most of the > time), we make note of those errors and then do a second review where > we ignore them (because we've already dealt with them). I agree - if a branch is *that* distracting you've every right to bounce it back. I hope mine aren't :) Cheers. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
