Re: [PATCH] Documentation/CommunityGuidelines

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/11/2013 08:29 PM, John Keeping wrote:
> On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>> * When reviewing other peoples' code, be tactful and constructive.  Set
>>> high expectations, but do what you can to help the submitter achieve
>>> them.  Don't demand changes based only on your personal preferences.
>>> Don't let the perfect be the enemy of the good.
>>
>> I think this is 30% aimed at me (as I think I do about that much of
>> the reviews around here).  I fully agree with most of them, but the
>> last sentence is a bit too fuzzy to be a practically useful
>> guideline.  Somebody's bare minimum is somebody else's perfection.
>> An unqualified "perfect is the enemy of good" is often incorrectly
>> used to justify "It works for me." and "There already are other
>> codepaths that do it in the same wrong way.", both of which make
>> things _worse_ for the long term project health.
> 
> One thing that I think is missing from these proposals so far is some
> clear indication that a review should not be confrontational.  Consider
> the following two review comments (taken from a recent example that
> happened to stick in my mind, but I don't want to single out any one
> individual here):
> 
>     Ugh, why this roundabout-passive-past tone?  Use imperative tone
>     like this:
> 
>         ...
> 
> vs.
> 
>     We normally use the imperative in commit messages, perhaps like
>     this?
> 
>         ...
> 
> Both say the same thing but the first immediately puts the submitter on
> the defensive.  If I see something like that on one of my patches I have
> to consciously resist the urge to reply immediately and instead review
> what I'm about to send once I've calmed down.

That's a very good point (and a good illustration, too).  How do you
like the new second and third sentences below?

* When reviewing other peoples' code, be tactful and constructive.
Remember that submitting patches for public critique can be very
intimidating and when mistakes are found it can be embarrassing.  Do
what you can to make it a positive and pleasant experience for the
submitter.  Set high expectations, but do what you can to help the
submitter achieve them.  Don't demand changes based only on your
personal preferences. Don't let the perfect be the enemy of the good.

(As Junio pointed out, the last sentence is not so great and a better
replacement would be welcome.)

> As my mother would say, "politeness costs nothing" ;-)

Does your mother program C?  We could use her around here :-)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/