Open Stack

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

> >
> >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
> >>
> >
> >__________________________________________________________________________
> >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