Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice
On 1/15/20 12:22 PM, Lars Kurth wrote: > @George, @Ian: let me know whether this is better and addresses your > concerns This looks good to me. One side note: > The way how a reviewer expresses feedback, has a big impact on how the author "The way how X happens" isn't grammatically correct; it's actually redundant. You can say, "The way a reviewer expresses feedback" (no "how"); or if that seems ambiguous for some reason, you can say, "The way *that* a reviewer expresses feedback", or "The way *in which* a reviewer expresses feedback...". Alternately, you could say "How a reviewer expresses feedback has a big impact..." -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice
On 15/01/2020, 10:47, "George Dunlap" wrote: On 1/13/20 9:21 PM, Lars Kurth wrote: > > > On 13/01/2020, 19:54, "George Dunlap" wrote: > > > > On Dec 30, 2019, at 7:32 PM, 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 > > > > +### Avoid opinion: stick to the facts > > In my talk on this subject I said “Avoid *inflammatory language*”. At some level it’s good to have strong opinions on what code should look like. It’s not opinions that are a problem, or even expressing opinions, but expressing them in a provocative or inflammatory way. > > Let me look at this again: I don't feel strongly about it > > I changed the title because I felt that the bulk of the > example is actually about sticking to the facts an opinion > and the inflammatory element was secondary. So it felt more > natural to me to change the title. Right; the point though specifically is that people's natural, and probably healthy response to poorly-written code, or to inconsiderately-written patch series in any way, is to use charged language. I wouldn't call any code "garbage", but code submitted is sometimes actually terrible, fragile, spaghetti, inefficient, racy, messy -- whatever bad things you can say about it -- and any well-trained developer will have the same opinion. [snip] I think people should be able to pick up what we mean from the reasoning and from the examples. I attached a conversation log on IRC and a diff against this snippet of the code for a resolution ‹lars_kurth› gwd: I just read your feedback on the CoC. I now agree with your argument that "Avoid opinion: stick to the facts" is a bad heading for that section ‹lars_kurth› gwd: however I still dont like “Avoid *inflammatory language*” - I am wondering whether "Avoid language that triggers a negative response" would be better ‹gwd› What is it you don't like about "inflammatory"? ‹lars_kurth› Also, I think I need to re-write some of the bridging paragraphs to fit the title ‹gwd› (Not arguing for 'inflammatory' per se, but knowing what you don't like about it helps if I'm trying to find an alternative) ‹lars_kurth› Firstly it is now somewhat politically charged (in some cultures), secondly I am not sure how well it translates and how clear it is to non-native english speakers ‹gwd› Any opinions on the other words I suggested? ‹lars_kurth› Provocative seems ok * Diziet reads the thread. ‹lars_kurth› "charged"? "loaded"? seems too generic ‹lars_kurth› "derogatory"? "contemptuous"? seems to be too harsh and infer too much bad intent ‹Diziet› "avoid ... emotive" maybe ? [11:18:15][11:18:31] ‹Diziet› "avoid derogatory or emotive language" ? ‹lars_kurth› Diziet, gwd: I think emotive is good and we can add derogatory ‹gwd› Doesn't "emotive" include positive emotions? "This patch is amazing, thank you" is a lot better than "This patch effictively simplifies this codebase very well, thank you". :-) ‹lars_kurth› That is true ‹lars_kurth› The same would be true for charged and loaded ‹Diziet› gwd: Hrm ‹Diziet› To be unambiguous I think only "negatively charged" will do. You can't have "negatively emotive" or some such. ‹Diziet› You could say "avoid emotive criticism" ‹gwd› I feel like "charged" is used more often for negative things. ‹lars_kurth› OK. Let's stick with Inflammatory and I can replace "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." in the first paragraph with a sentence that clarifies that the intention is to avoid triggering negativity ‹lars_kurth› I am going to draft some text for this section and send it in response rather than doing a new version for now ‹gwd› + ‹Diziet› I think `derogatory' and `emotive criticism' and `negatively charged' are all better than `inflammatory'. ‹Diziet› But `inflammatory' will do. ‹lars_kurth› The section as it is comes across as a little clumsy (in that it doesn't flow well ‹lars_kurth› As an aside: does anyone know how I can redact text in markdown? I guess I can just add "" for words I dont want to show ‹Diziet› https://stackoverflow.com/questions/4823468/comments-in-markdown ‹gwd› lars_kurth: That's what I would do. (Although I would use [], which are more traditional for edits to quoted text.) ‹Diziet› Oh I see, we're talking about the oebxra gbrf thing ‹Diziet› I would just write literally [redacted]. ‹Diziet› Or rot13 it as I just did but the audience of the CoC will have no idea what that is even if you add a
Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice
On 1/13/20 9:21 PM, Lars Kurth wrote:
>
>
> On 13/01/2020, 19:54, "George Dunlap" wrote:
>
>
> > On Dec 30, 2019, at 7:32 PM, 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
> >
> > +### Avoid opinion: stick to the facts
>
> In my talk on this subject I said “Avoid *inflammatory language*”. At
> some level it’s good to have strong opinions on what code should look like.
> It’s not opinions that are a problem, or even expressing opinions, but
> expressing them in a provocative or inflammatory way.
>
> Let me look at this again: I don't feel strongly about it
>
> I changed the title because I felt that the bulk of the
> example is actually about sticking to the facts an opinion
> and the inflammatory element was secondary. So it felt more
> natural to me to change the title.
Right; the point though specifically is that people's natural, and
probably healthy response to poorly-written code, or to
inconsiderately-written patch series in any way, is to use charged
language. I wouldn't call any code "garbage", but code submitted is
sometimes actually terrible, fragile, spaghetti, inefficient, racy,
messy -- whatever bad things you can say about it -- and any
well-trained developer will have the same opinion.
It's not a problem at all to have opinions on code; I think that's a
prerequisite for being a good developer. It's also not a problem at all
to say, "This code is great" or something positive about the submitter;
nor is it a problem to talk *together* about something not written by
the submitter ("Wow, this code you're trying to fix is a mess.") The
point specifically is to avoid things which are likely to provoke a
negative emotional response in the submitter.
> But then looking at the definition of inflammatory language,
> aka "an inflammatory question or an inflammatory statement
> would be one which would somehow predispose the listeners
> towards a subject in an unreasonable, prejudiced way."
> It is clearly also true that the example is inflammatory.
>
> I think I may have tripped over an area where there is no good
> language match: the German translations of inflammatory
> aufrührerisch & aufwieglerisch have an element of rebellion
> and mischief to them (at least when I grew up).
"Provocative"? "charged"? "loaded"? "derogatory"? "contemptuous"?
> I am wondering though, whether it is necessary to include
> a definition of an inflammatory question or an inflammatory
> statement if we stick with it in the title
I think people should be able to pick up what we mean from the reasoning
and from the examples.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice
On 13/01/2020, 19:54, "George Dunlap" wrote: > On Dec 30, 2019, at 7:32 PM, 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 > > +### Avoid opinion: stick to the facts In my talk on this subject I said “Avoid *inflammatory language*”. At some level it’s good to have strong opinions on what code should look like. It’s not opinions that are a problem, or even expressing opinions, but expressing them in a provocative or inflammatory way. Let me look at this again: I don't feel strongly about it I changed the title because I felt that the bulk of the example is actually about sticking to the facts an opinion and the inflammatory element was secondary. So it felt more natural to me to change the title. But then looking at the definition of inflammatory language, aka "an inflammatory question or an inflammatory statement would be one which would somehow predispose the listeners towards a subject in an unreasonable, prejudiced way." It is clearly also true that the example is inflammatory. I think I may have tripped over an area where there is no good language match: the German translations of inflammatory aufrührerisch & aufwieglerisch have an element of rebellion and mischief to them (at least when I grew up). I am wondering though, whether it is necessary to include a definition of an inflammatory question or an inflammatory statement if we stick with it in the title > > +> Foot binding was the custom of applying tight binding to the feet of young > +> girls to modify the shape and size of their feet. ... foot binding was a > +> painful practice and significantly limited the mobility of women, resulting > +> in lifelong disabilities for most of its subjects. ... Binding usually > +> started during the winter months since the feet were more likely to be numb, > +> and therefore the pain would not be as extreme. …The toes on each foot > +> were curled under, then pressed with great force downwards and squeezed > +> into the sole of the foot until the toes broke… In my talk I covered the last three words behind a blue square, since this image is pretty violent — and is gendered violence at that. Some people joke about “triggering”, but there are certainly people who have experienced violence, who when they come across descriptions of it unexpectedly suddenly have loads of unwelcome emotions to deal with; and I venture to guess that most people skimming through such a guide wouldn’t be expecting to come across something like this. Personally I would replace the last three words with [redacted]. The point can be made without being so explicit. Anyone who wants to know what happens can go look up the entry themselves. OK. I can do that. I copied the text from the content outline on slide share and wasn't even looking at the slides themselves Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice
> On Dec 30, 2019, at 7:32 PM, 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 > > +### Avoid opinion: stick to the facts In my talk on this subject I said “Avoid *inflammatory language*”. At some level it’s good to have strong opinions on what code should look like. It’s not opinions that are a problem, or even expressing opinions, but expressing them in a provocative or inflammatory way. > > +> Foot binding was the custom of applying tight binding to the feet of young > +> girls to modify the shape and size of their feet. ... foot binding was a > +> painful practice and significantly limited the mobility of women, resulting > +> in lifelong disabilities for most of its subjects. ... Binding usually > +> started during the winter months since the feet were more likely to be > numb, > +> and therefore the pain would not be as extreme. …The toes on each foot > +> were curled under, then pressed with great force downwards and squeezed > +> into the sole of the foot until the toes broke… In my talk I covered the last three words behind a blue square, since this image is pretty violent — and is gendered violence at that. Some people joke about “triggering”, but there are certainly people who have experienced violence, who when they come across descriptions of it unexpectedly suddenly have loads of unwelcome emotions to deal with; and I venture to guess that most people skimming through such a guide wouldn’t be expecting to come across something like this. Personally I would replace the last three words with [redacted]. The point can be made without being so explicit. Anyone who wants to know what happens can go look up the entry themselves. Everything else looks good! -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice
On 30.12.19 20:32, 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 Changes since v3 * Fixed typo Changes since v2 (added in v2) * Fix typos * Extended "Verbose vs. terse" * Added "Clarity over Verbosity" * Broke "Identify the severity of an issue or disagreement" into two chapters - "Identify the severity and optionality of review comments" and made clarifications - "Identify the severity of a disagreement" - Expanded "Prioritize significant flaws" * Added "Reviewers: Take account of previous reviewer(s) comments" * Added prefixes such as "Reviewers:" where appropriate * Fixed lien wrapping to 80 characters * Replaced inline links with reference links 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 | 504 ++ 1 file changed, 504 insertions(+) create mode 100644 communication-practice.md diff --git a/communication-practice.md b/communication-practice.md new file mode 100644 index 000..438b73a --- /dev/null +++ b/communication-practice.md @@ -0,0 +1,504 @@ +# 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 add "are" + all trying to create a productive, welcoming and agile environment, we do + not always succeed. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v4 6/7] 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 Changes since v3 * Fixed typo Changes since v2 (added in v2) * Fix typos * Extended "Verbose vs. terse" * Added "Clarity over Verbosity" * Broke "Identify the severity of an issue or disagreement" into two chapters - "Identify the severity and optionality of review comments" and made clarifications - "Identify the severity of a disagreement" - Expanded "Prioritize significant flaws" * Added "Reviewers: Take account of previous reviewer(s) comments" * Added prefixes such as "Reviewers:" where appropriate * Fixed lien wrapping to 80 characters * Replaced inline links with reference links 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 | 504 ++ 1 file changed, 504 insertions(+) create mode 100644 communication-practice.md diff --git a/communication-practice.md b/communication-practice.md new file mode 100644 index 000..438b73a --- /dev/null +++ b/communication-practice.md @@ -0,0 +1,504 @@ +# 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 the 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
