Open Stack

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. :)

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
>