launchpad-dev team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

On Thursday 21 October 2010 11:32:29 Graham Binns wrote:
> On 21 October 2010 11:20, Julian Edwards <julian.edwards@xxxxxxxxxxxxx> 
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.