Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On 27/11/2019, 19:06, "Stefano Stabellini" wrote: 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. Yes. Ultimately the effect of a better communication should overall be a net-positive in terms of effort. Maybe we should highlight and encourage "clarity" instead of "verbosity" of the communication, and encourage "expressing appreciation" to newcomers, not necessarily to seasoned contributors. Good idea 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. It is worth saying this. Regards Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On 27/11/2019, 18:57, "Stefano Stabellini" wrote: On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This guide covers the bulk on Best Practice related to code review > It primarily focusses on code review interactions > It also covers how to deal with Misunderstandings and Cultural > Differences > > Signed-off-by: Lars Kurth > --- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@xenproject.org > --- > communication-practice.md | 410 ++ > 1 file changed, 410 insertions(+) > create mode 100644 communication-practice.md > > diff --git a/communication-practice.md b/communication-practice.md > new file mode 100644 > index 000..db9a5ef > --- /dev/null > +++ b/communication-practice.md > @@ -0,0 +1,410 @@ > +# Communication Best Practice > + > +This guide provides communication Best Practice that helps you in > +* Using welcoming and inclusive language > +* Keeping discussions technical and actionable > +* Being respectful of differing viewpoints and experiences > +* Being aware of your own and counterpart’s communication style and culture > +* Show empathy towards other community members > + > +## Code reviews for **reviewers** and **patch authors** > + > +Before embarking on a code review, it is important to remember that > +* A poorly executed code review can hurt the contributors feeling, even when a reviewer > + did not intend to do so. Feeling defensive is a normal reaction to a critique or feedback. > + A reviewer should be aware of how the pitch, tone, or sentiment of their comments > + could be interpreted by the contributor. The same applies to responses of an author > + to the reviewer. > +* When reviewing someone's code, you are ultimately looking for issues. A good code > + reviewer is able to mentally separate finding issues from articulating code review > + comments in a constructive and positive manner: depending on your personality this > + can be **difficult** and you may need to develop a technique that works for you. > +* As software engineers we like to be proud of the solutions we came up with. This can > + make it easy to take another people’s criticism personally. Always remember that it is > + the code that is being reviewed, not you as a person. > +* When you receive code review feedback, please be aware that we have reviewers > + from different backgrounds, communication styles and cultures. Although we all trying > + to create a productive, welcoming and agile environment, we do not always succeed. > + > +### 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 ^ the top > +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
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This guide covers the bulk on Best Practice related to code review > It primarily focusses on code review interactions > It also covers how to deal with Misunderstandings and Cultural > Differences > > Signed-off-by: Lars Kurth > --- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@xenproject.org > --- > communication-practice.md | 410 > ++ > 1 file changed, 410 insertions(+) > create mode 100644 communication-practice.md > > diff --git a/communication-practice.md b/communication-practice.md > new file mode 100644 > index 000..db9a5ef > --- /dev/null > +++ b/communication-practice.md > @@ -0,0 +1,410 @@ > +# Communication Best Practice > + > +This guide provides communication Best Practice that helps you in > +* Using welcoming and inclusive language > +* Keeping discussions technical and actionable > +* Being respectful of differing viewpoints and experiences > +* Being aware of your own and counterpart’s communication style and culture > +* Show empathy towards other community members > + > +## Code reviews for **reviewers** and **patch authors** > + > +Before embarking on a code review, it is important to remember that > +* A poorly executed code review can hurt the contributors feeling, even when > a reviewer > + did not intend to do so. Feeling defensive is a normal reaction to a > critique or feedback. > + A reviewer should be aware of how the pitch, tone, or sentiment of their > comments > + could be interpreted by the contributor. The same applies to responses of > an author > + to the reviewer. > +* When reviewing someone's code, you are ultimately looking for issues. A > good code > + reviewer is able to mentally separate finding issues from articulating > code review > + comments in a constructive and positive manner: depending on your > personality this > + can be **difficult** and you may need to develop a technique that works > for you. > +* As software engineers we like to be proud of the solutions we came up > with. This can > + make it easy to take another people’s criticism personally. Always > remember that it is > + the code that is being reviewed, not you as a person. > +* When you receive code review feedback, please be aware that we have > reviewers > + from different backgrounds, communication styles and cultures. Although we > all trying > + to create a productive, welcoming and agile environment, we do not always > succeed. > + > +### 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 ^ the top > +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. > + > +### Avoid opinion: stick to the facts > +The way how a reviewer expresses feedback, has a big impact on how the author > +perceives the feedback. Key to this is what we call **stick to the facts**. >
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On 27/09/2019, 11:17, "Lars Kurth" wrote: On 27/09/2019, 10:14, "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. 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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On 27/09/2019, 10:14, "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. 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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On 27/09/2019, 09:59, "Jan Beulich" 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
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [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-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
From: Lars Kurth This guide covers the bulk on Best Practice related to code review It primarily focusses on code review interactions It also covers how to deal with Misunderstandings and Cultural Differences Signed-off-by: Lars Kurth --- Cc: minios-de...@lists.xenproject.org Cc: xen-...@lists.xenproject.org Cc: win-pv-de...@lists.xenproject.org Cc: mirageos-de...@lists.xenproject.org Cc: committ...@xenproject.org --- communication-practice.md | 410 ++ 1 file changed, 410 insertions(+) create mode 100644 communication-practice.md diff --git a/communication-practice.md b/communication-practice.md new file mode 100644 index 000..db9a5ef --- /dev/null +++ b/communication-practice.md @@ -0,0 +1,410 @@ +# Communication Best Practice + +This guide provides communication Best Practice that helps you in +* Using welcoming and inclusive language +* Keeping discussions technical and actionable +* Being respectful of differing viewpoints and experiences +* Being aware of your own and counterpart’s communication style and culture +* Show empathy towards other community members + +## Code reviews for **reviewers** and **patch authors** + +Before embarking on a code review, it is important to remember that +* A poorly executed code review can hurt the contributors feeling, even when a reviewer + did not intend to do so. Feeling defensive is a normal reaction to a critique or feedback. + A reviewer should be aware of how the pitch, tone, or sentiment of their comments + could be interpreted by the contributor. The same applies to responses of an author + to the reviewer. +* When reviewing someone's code, you are ultimately looking for issues. A good code + reviewer is able to mentally separate finding issues from articulating code review + comments in a constructive and positive manner: depending on your personality this + can be **difficult** and you may need to develop a technique that works for you. +* As software engineers we like to be proud of the solutions we came up with. This can + make it easy to take another people’s criticism personally. Always remember that it is + the code that is being reviewed, not you as a person. +* When you receive code review feedback, please be aware that we have reviewers + from different backgrounds, communication styles and cultures. Although we all trying + to create a productive, welcoming and agile environment, we do not always succeed. + +### 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. + +### Avoid opinion: stick to the facts +The way how a reviewer expresses feedback, has a big impact on how the author +perceives the feedback. Key to this is what we call **stick to the facts**. The same is +true when a patch author is responding to a comment from a reviewer. + +One of our maintainers has been studying Mandarin for several years and has come +across the most strongly-worded dictionary entry +[he has ever seen](https://youtu.be/ehZvBmrLRwg?t=834). This example +illustrates the problem of using
