Mailing List Archive

Re: [PATCH v2 5/6] Add guide on Communication Best Practice
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-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
On 26.09.2019 21:39, Lars Kurth wrote:
> +### Verbose vs. terse
> +Due to the time it takes to review and compose code reviewer, reviewers often adopt a
> +terse style. It is not unusual to see review comments such as
> +> typo
> +> s/resions/regions/
> +> coding style
> +> coding style: brackets not needed
> +etc.
> +
> +Terse code review style has its place and can be productive for both the reviewer and
> +the author. However, overuse can come across as unfriendly, lacking empathy and
> +can thus create a negative impression with the author of a patch. This is in particular
> +true, when you do not know the author or the author is a newcomer. Terse
> +communication styles can also be perceived as rude in some cultures.

And another remark here: Not being terse in situations like the ones
enumerated as examples above is a double waste of the reviewer's time:
They shouldn't even need to make such comments, especially not many
times for a single patch (see your mention of "overuse"). I realize
we still have no automated mechanism to check style aspects, but
anybody can easily look over their patches before submitting them.
And for an occasional issue I think a terse reply is quite reasonable
to have.

Overall I'm seeing the good intentions of this document, yet I'd still
vote at least -1 on it if it came to a vote. Following even just a
fair part of it is a considerable extra amount of time to invest in
reviews, when we already have a severe reviewing bottleneck. If I have
to judge between doing a bad (stylistically according to this doc, not
technically) review or none at all (because of time constraints), I'd
favor the former. Unless of course I'm asked to stop doing so, in
which case I'd expect whoever asks to arrange for the reviews to be
done by someone else in due course.

I'm sorry for (likely) sounding destructive here.

Jan

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
?On 27/09/2019, 09:59, "Jan Beulich" <jbeulich@suse.com> 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-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
?On 27/09/2019, 10:14, "Jan Beulich" <jbeulich@suse.com> wrote:

On 26.09.2019 21:39, Lars Kurth wrote:
> +### Verbose vs. terse
> +Due to the time it takes to review and compose code reviewer, reviewers often adopt a
> +terse style. It is not unusual to see review comments such as
> +> typo
> +> s/resions/regions/
> +> coding style
> +> coding style: brackets not needed
> +etc.
> +
> +Terse code review style has its place and can be productive for both the reviewer and
> +the author. However, overuse can come across as unfriendly, lacking empathy and
> +can thus create a negative impression with the author of a patch. This is in particular
> +true, when you do not know the author or the author is a newcomer. Terse
> +communication styles can also be perceived as rude in some cultures.

And another remark here: Not being terse in situations like the ones
enumerated as examples above is a double waste of the reviewer's time:
They shouldn't even need to make such comments, especially not many
times for a single patch (see your mention of "overuse"). I realize
we still have no automated mechanism to check style aspects, but
anybody can easily look over their patches before submitting them.
And for an occasional issue I think a terse reply is quite reasonable
to have.

At the end of the day, none if this is mandatory. The document also
has two audiences
* Authors which get review feedback : for example by just having
this section in there it helps

I added this section primarily because we do see the occasional
very terse review style and even I think sometimes: wow, that comes
across as harsh. But I also know, that it isn't intentional and that
I have a fairly thick skin. And it is not exclusive to typos and minor issues.

What I was trying to do in this document is to provide
a guide which shows the different patterns from both perspectives.
I hope I succeeded in this, but I believe that you primarily
reviewed the document from the view point of a code reviewer.

Overall I'm seeing the good intentions of this document, yet I'd still
vote at least -1 on it if it came to a vote. Following even just a
fair part of it is a considerable extra amount of time to invest in
reviews, when we already have a severe reviewing bottleneck. If I have
to judge between doing a bad (stylistically according to this doc, not
technically) review or none at all (because of time constraints), I'd
favor the former. Unless of course I'm asked to stop doing so, in
which case I'd expect whoever asks to arrange for the reviews to be
done by someone else in due course.

First of all: this would be our gold standard and as pointed out earlier
So it is intended to provide the tools to do better: for example, from
my point of view if you followed some of it for example for newcomers
and sparingly when you feel it is right, that would already be a
win-win. Also, consider that a more positive tone should also have the
effect that there may be less unnecessary discussion. I think this
is particularly true when it comes to the sections on fact-based
responses vs. some which are unclear. Unfortunately, I don't have data
on this to prove it.

Can I maybe get you to reconsider and re-review the next version from the
view point of an author and maybe make suggestions on how to create more
balance

I'm sorry for (likely) sounding destructive here.

I don't see this your feedback as destructive and do hope that I
can convince you that documenting some of the patterns which
happen on the list are in fact a net-positive

Regards
Lars

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
?On 27/09/2019, 11:17, "Lars Kurth" <lars.kurth@citrix.com> wrote:



On 27/09/2019, 10:14, "Jan Beulich" <jbeulich@suse.com> wrote:

On 26.09.2019 21:39, Lars Kurth wrote:
> +### Verbose vs. terse
> +Due to the time it takes to review and compose code reviewer, reviewers often adopt a
> +terse style. It is not unusual to see review comments such as
> +> typo
> +> s/resions/regions/
> +> coding style
> +> coding style: brackets not needed
> +etc.
> +
> +Terse code review style has its place and can be productive for both the reviewer and
> +the author. However, overuse can come across as unfriendly, lacking empathy and
> +can thus create a negative impression with the author of a patch. This is in particular
> +true, when you do not know the author or the author is a newcomer. Terse
> +communication styles can also be perceived as rude in some cultures.

And another remark here: Not being terse in situations like the ones
enumerated as examples above is a double waste of the reviewer's time:
They shouldn't even need to make such comments, especially not many
times for a single patch (see your mention of "overuse"). I realize
we still have no automated mechanism to check style aspects, but
anybody can easily look over their patches before submitting them.
And for an occasional issue I think a terse reply is quite reasonable
to have.

At the end of the day, none if this is mandatory. The document also
has two audiences
* Authors which get review feedback : for example by just having
this section in there it helps

This was meant to read: it helps set expectations and promotes
understanding for some of the patterns used

I added this section primarily because we do see the occasional
very terse review style and even I think sometimes: wow, that comes
across as harsh. But I also know, that it isn't intentional and that
I have a fairly thick skin. And it is not exclusive to typos and minor issues.

Lars

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
On 27.09.2019 12:17, Lars Kurth wrote:
> Can I maybe get you to reconsider and re-review the next version from the
> view point of an author and maybe make suggestions on how to create more
> balance

I'll certainly make an attempt.

Jan

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
On 9/26/19 8:39 PM, Lars Kurth wrote:
> +investigate the practice foot-binding, it is hard to disagree with the dictionart entry.

Typo: dictionary

-George

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
On 9/27/19 10:14 AM, Jan Beulich wrote:
> On 26.09.2019 21:39, Lars Kurth wrote:
>> +### Verbose vs. terse
>> +Due to the time it takes to review and compose code reviewer, reviewers often adopt a
>> +terse style. It is not unusual to see review comments such as
>> +> typo
>> +> s/resions/regions/
>> +> coding style
>> +> coding style: brackets not needed
>> +etc.
>> +
>> +Terse code review style has its place and can be productive for both the reviewer and
>> +the author. However, overuse can come across as unfriendly, lacking empathy and
>> +can thus create a negative impression with the author of a patch. This is in particular
>> +true, when you do not know the author or the author is a newcomer. Terse
>> +communication styles can also be perceived as rude in some cultures.
>
> And another remark here: Not being terse in situations like the ones
> enumerated as examples above is a double waste of the reviewer's time:

FWIW I don't think this document prohibits terse replies. It points out
that they can come across as unfriendly, and they can be perceived as
rude in some cultures; both of which are true. It then *recommends*
that reviewers compensate for it in a review opening (i.e., once per
patch / series) which expresses appreciation; which is both helpful and
relatively low cost.

The point of the opening is to set the tone. If you start out with
something positive, and ends with "thanks", then a long series of terse
comments is more likely to be read as simply being efficient. If the
entire review consists of nothing but criticism or terse comments, it's
more likely to be read as annoyance on the part of the reviewer.

> They shouldn't even need to make such comments, especially not many
> times for a single patch (see your mention of "overuse"). I realize
> we still have no automated mechanism to check style aspects, but
> anybody can easily look over their patches before submitting them.
> And for an occasional issue I think a terse reply is quite reasonable
> to have.

This sort of sounds like you are *intending* to express annoyance?

If so, that's a slightly different question than what this section is
addressing. :-)

-George

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
On 07.10.2019 18:13, George Dunlap wrote:
> On 9/27/19 10:14 AM, Jan Beulich wrote:
>> On 26.09.2019 21:39, Lars Kurth wrote:
>>> +### Verbose vs. terse
>>> +Due to the time it takes to review and compose code reviewer, reviewers often adopt a
>>> +terse style. It is not unusual to see review comments such as
>>> +> typo
>>> +> s/resions/regions/
>>> +> coding style
>>> +> coding style: brackets not needed
>>> +etc.
>>> +
>>> +Terse code review style has its place and can be productive for both the reviewer and
>>> +the author. However, overuse can come across as unfriendly, lacking empathy and
>>> +can thus create a negative impression with the author of a patch. This is in particular
>>> +true, when you do not know the author or the author is a newcomer. Terse
>>> +communication styles can also be perceived as rude in some cultures.
>>
>> And another remark here: Not being terse in situations like the ones
>> enumerated as examples above is a double waste of the reviewer's time:
>
> FWIW I don't think this document prohibits terse replies. It points out
> that they can come across as unfriendly, and they can be perceived as
> rude in some cultures; both of which are true. It then *recommends*
> that reviewers compensate for it in a review opening (i.e., once per
> patch / series) which expresses appreciation; which is both helpful and
> relatively low cost.
>
> The point of the opening is to set the tone. If you start out with
> something positive, and ends with "thanks", then a long series of terse
> comments is more likely to be read as simply being efficient. If the
> entire review consists of nothing but criticism or terse comments, it's
> more likely to be read as annoyance on the part of the reviewer.
>
>> They shouldn't even need to make such comments, especially not many
>> times for a single patch (see your mention of "overuse"). I realize
>> we still have no automated mechanism to check style aspects, but
>> anybody can easily look over their patches before submitting them.
>> And for an occasional issue I think a terse reply is quite reasonable
>> to have.
>
> This sort of sounds like you are *intending* to express annoyance?

Implicitly by being terse, yes. I've been trying to avoid expressing
such explicitly, but I have to admit there are (luckily only few)
cases where I find it pretty hard to stay away.

Jan

_______________________________________________
Xen-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api
Re: [PATCH v2 5/6] Add guide on Communication Best Practice [ In reply to ]
On Fri, 27 Sep 2019, Jan Beulich wrote:
> On 26.09.2019 21:39, Lars Kurth wrote:
> > +### Verbose vs. terse
> > +Due to the time it takes to review and compose code reviewer, reviewers often adopt a
> > +terse style. It is not unusual to see review comments such as
> > +> typo
> > +> s/resions/regions/
> > +> coding style
> > +> coding style: brackets not needed
> > +etc.
> > +
> > +Terse code review style has its place and can be productive for both the reviewer and
> > +the author. However, overuse can come across as unfriendly, lacking empathy and
> > +can thus create a negative impression with the author of a patch. This is in particular
> > +true, when you do not know the author or the author is a newcomer. Terse
> > +communication styles can also be perceived as rude in some cultures.
>
> And another remark here: Not being terse in situations like the ones
> enumerated as examples above is a double waste of the reviewer's time:
> They shouldn't even need to make such comments, especially not many
> times for a single patch (see your mention of "overuse"). I realize
> we still have no automated mechanism to check style aspects, but
> anybody can easily look over their patches before submitting them.
> And for an occasional issue I think a terse reply is quite reasonable
> to have.
>
> Overall I'm seeing the good intentions of this document, yet I'd still
> vote at least -1 on it if it came to a vote. Following even just a
> fair part of it is a considerable extra amount of time to invest in
> reviews, when we already have a severe reviewing bottleneck. If I have
> to judge between doing a bad (stylistically according to this doc, not
> technically) review or none at all (because of time constraints), I'd
> favor the former. Unless of course I'm asked to stop doing so, in
> which case I'd expect whoever asks to arrange for the reviews to be
> done by someone else in due course.

Reading the document, I think Jan has a point that it gives the
impression that following the suggestions would take significant
efforts, while actually I don't think Lars meant it that way at all, and
I don't think it should be the case either.

Maybe we should highlight and encourage "clarity" instead of "verbosity"
of the communication, and encourage "expressing appreciation" to
newcomers, not necessarily to seasoned contributors.

The ultimate goal of this document is actually to *reduce* our overall
efforts by making our communication more efficient, not to increase
efforts. Maybe it is worth saying this too.

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