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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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.

Jan

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