[openstack-dev] [Glance] Nitpicking in code reviews

Flavio Percoco flavio at redhat.com
Fri Mar 13 14:23:52 UTC 2015


On 13/03/15 10:36 +0100, Gorka Eguileor wrote:
>On Thu, Mar 12, 2015 at 10:58:37AM -0700, Jim Rollenhagen wrote:
>> On Thu, Mar 12, 2015 at 09:07:30AM -0500, Flavio Percoco wrote:
>> > On 11/03/15 15:06 -1000, John Bresnahan wrote:
>> > >FWIW I agree with #3 and #4 but not #1 and #2.  Spelling is an easy enough
>> > >thing to get right and speaks to the quality standard to which the product
>> > >is held even in commit messages and comments (consider the 'broken window
>> > >theory').  Of course everyone makes mistakes (I am a terrible speller) but
>> > >correcting a spelling error should be a trivial matter.  If a reviewer
>> > >notices a spelling error I would expect them to point it.
>> >
>> > I'd agree depending on the status of the patch. If the patch has
>> > already 2 +2s and someone blocks it because of a spelling error, then
>> > the cost of fixing it, running the CI jobs and getting the reviews
>> > again is higher than living with a simple typo.
>> >
>> > Process and rules are good but we must evaluate them in a case by case
>> > basis to make sure we're not blocking important work on things that
>> > are not that relevant after all.
>> >
>>
>> In Ironic, we've set up rough guidelines for situations like these.
>>
>> 1) Reviewers should not -1 a patch solely for spelling/grammar errors.
>> 2) If a reviewer finds one of these errors and feels strongly that it
>>    should be corrected, they should do one of the following:
>>    2a) If it won't slow down the patch (i.e. no +2's on it), fix it
>>        themselves and submit a new patchset. This is even easier for
>>        commit messages; they can be edited directly in Gerrit.
>>    2b) Make a note on the review and push a follow-up patch to fix it.
>>    2c) Ask the submitter for a follow-up patch that fixes it.
>>
>> This has dramatically reduced our nitpicking on patches and has even
>> seemed to improve our general velocity.
>>
>> // jim
>>
>> > >
>
>I like Jim's idea of having some rough guidelines as this will make it
>easier for newcomers, like myself, get an idea of how we should vote.
>Otherwise we infer them, maybe even incorrectly, from other reviews and
>end up -1 patches just for grammar errors in comments, something we
>wouldn't have usually done.
>
>I'm glad to know that all things mentioned by Erno are not really worth
>a -1. :)

For the sake of nitpicking, more than a policy, this should be a guide
to reviewing and shouldn't be just for Glance. This knowledge is
something the community has been working on over the years and I
believe it's useful for everyone.

As you mentioned, it'd helps newcomers to understand the reviewing
process and it'll help us to educate not just newcomers but old
members too ;)

Cheers,
Fla

>
>Gorka.
>
>> > >On 3/11/15 2:22 PM, Kuvaja, Erno wrote:
>> > >>Hi all,
>> > >>
>> > >>Following the code reviews lately I’ve noticed that we (the fan club
>> > >>seems to be growing on weekly basis) have been growing culture of
>> > >>nitpicking [1] and bikeshedding [2][3] over almost every single change.
>> > >>
>> > >>Seriously my dear friends, following things are not worth of “-1” vote
>> > >>if even a comment:
>> > >>
>> > >>1)Minor spelling errors on commit messages (as long as the message comes
>> > >>through and flags are not misspelled).
>> > >>
>> > >>2)Minor spelling errors on comments (docstrings and documentation is
>> > >>there and there, but comments, come-on).
>> > >>
>> > >>3)Used syntax that is functional, readable and does not break
>> > >>consistency but does not please your poem bowel.
>> > >>
>> > >>4)Other things you “just did not realize to check if they were there”.
>> > >>After you have gone through the whole change go and look your comments
>> > >>again and think twice if your concern/question/whatsoever was addressed
>> > >>somewhere else than where your first intuition would have dropped it.
>> > >>
>> > >>We have relatively high volume for glance at the moment and this
>> > >>nitpicking and bikeshedding does not help anyone. At best it just
>> > >>tightens nerves and breaks our group. Obviously if there is “you had ONE
>> > >>job” kind of situations or there is relatively high amount of errors
>> > >>combined with something serious it’s reasonable to ask fix the typos on
>> > >>the way as well. The reason being need to increase your statistics,
>> > >>personal perfectionist nature or actually I do not care what; just stop
>> > >>or go and do it somewhere else.
>> >
>> >
>> > Thanks for bringing all this up, Erno. I've been seeing the same
>> > pattern for all the points you've mentioned above. It's a good
>> > reminder for people to treat each patch individually so we avoid
>> > making our process and rules a pain for everyone.
>> >
>> > Flavio
>> >
>> > >>
>> > >>Love and pink ponies,
>> > >>
>> > >>-Erno
>> > >>
>> > >>[1] www.urbandictionary.com/define.php?term=nitpicking
>> > >><http://www.urbandictionary.com/define.php?term=nitpicking>
>> > >>
>> > >>[2] http://bikeshed.com
>> > >>
>> > >>[3] http://en.wiktionary.org/wiki/bikeshedding
>> > >>
>> > >>
>> > >__________________________________________________________________________
>> > >OpenStack Development Mailing List (not for usage questions)
>> > >Unsubscribe:
>> > >OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> > >http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> >
>> > --
>> > @flaper87
>> > Flavio Percoco
>>
>
>__________________________________________________________________________
>OpenStack Development Mailing List (not for usage questions)
>Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

-- 
@flaper87
Flavio Percoco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20150313/a29d606d/attachment.pgp>


More information about the OpenStack-dev mailing list