Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

On 27/09/2019, 09:59, "Jan Beulich" <jbeulich@xxxxxxxx> wrote:

    On 26.09.2019 21:39, Lars Kurth wrote:
    > +### Express appreciation
    > +As the nature of code review to find bugs and possible issues, it is 
very easy for
    > +reviewers to get into a mode of operation where the patch review ends up 
being a list
    > +of issues, not mentioning what is right and well done. This can lead to 
the code
    > +submitter interpreting your feedback in a negative way.
    > +
    > +The opening of a code review provides an opportunity to address this and 
also sets the
    > +tone for the rest of the code review. Starting **every** review on a 
positive note, helps
    > +set the tone for the rest of the review.
    > +
    > +For an initial patch, you can use phrases such as
    > +> Thanks for the patch
    > +> Thanks for doing this
    > +
    > +For further revisions within a review, phrases such as
    > +> Thank you for addressing the last set of changes
    > +
    > +If you believe the code was good, it is good practice to highlight this 
by using phrases
    > +such as
    > +> Looks good, just a few comments
    > +> The changes you have made since the last version look good
    > +
    > +If you think there were issues too many with the code to use one of the 
phrases,
    > +you can still start on a positive note, by for example saying
    > +> I think this is a good change
    > +> I think this is a good feature proposal
    > +
    > +It is also entirely fine to highlight specific changes as good. The best 
place to
    > +do this, is at top of a patch, as addressing code review comments 
typically requires
    > +a contributor to go through the list of things to address and an 
in-lined positive
    > +comment is likely to break that workflow.
    > +
    > +You should also consider, that if you review a patch of an experienced
    > +contributor phrases such as *Thanks for the patch* could come across as
    > +patronizing, while using *Thanks for doing this* is less likely to be 
interpreted
    > +as such.
    > +
    > +Appreciation should also be expressed by patch authors when asking for 
clarifications
    > +to a review or responding to questions. A simple
    > +> Thank you for your feedback
    > +> Thank you for your reply
    > +> Thank you XXX!
    > +
    > +is normally sufficient.
    
    To all of this I can't resist giving a remark that I've already given
    when discussing the matter in person: I'm not sure about English, but
    in German the word "Phrase" also has an, at times very, negative
    meaning. When I get review feedback starting like suggested above, it
    definitely feels to me more like this (the statement was added there
    just for it to be there). I realize this may not always (and perhaps
    even in a majority of situations) be the case, but that's how it feels
    to me nevertheless. As a result I would rather rarely, if ever, start
    like this (on the basis of "don't do to others what you dislike
    yourself"); a case where I might do so would be when I had asked for
    (or offloaded) the putting together of a particular change.

I think your reply proves almost entirely the point of the article. In the
end all of this depends on communication styles (both personal and
cultural). My take to it is that there is a difference between

a) Someone you know: what ultimately will happen is that 
when you engage with someone you know and had done reviews before
you ultimately become more terse and also drop niceties.
Which is OK

b) Someone you don’t know: in that case, we should start from
a reasonable middle ground and put in a bit more effort

    Even worse, there have been (also very recent) examples where replies
    come back saying just "Thank you" (e.g. for an ack). Such certainly
    get sent with good intentions, but people doing so likely overlook
    the fact that there's already way too much email to read for many of
    us. (The same applies to other netiquette aspects that I keep
    mentioning on e.g. summits, but with apparently little to no effect:
    People frequently fail to strip unnecessary context when replying,
    requiring _every_ reader to scroll through a perhaps long mail just
    to find that there's almost nothing of interest. People also seem to
    have difficulty understanding the difference between To and Cc.)

That is a good point and I had forgotten about it
Thanks for reminding me

I can add a section on this which looks for balance in the interest
of saving your communication partner's time. Ultimately this is a
also showing a degree of thoughtfulness. 

And we can state in there things like the CC/TO list
And not to thank code reviewers for ACKs or otherwise in a 
stand-alone e-mail
    
    The bottom line of this is - the "being kind to one another" aspect
    of asking for this behavior needs to be weighed carefully against its
    effects of unduly consuming everybody's time.
    
I am fully aware of this, and was trying to approach this from this
viewpoint of trying to achieve a sensible balance

But after your comment, maybe that was not clear enough

Best Regards
Lars    

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel