Mailing List Archive

?On 13/01/2020, 19:54, "George Dunlap" <George.Dunlap@citrix.com> wrote:


> On Dec 30, 2019, at 7:32 PM, Lars Kurth <lars.kurth@xenproject.org> wrote:
>
> From: Lars Kurth <lars.kurth@citrix.com>
>
> 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-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api

On 1/13/20 9:21 PM, Lars Kurth wrote:
>
>
> ?On 13/01/2020, 19:54, "George Dunlap" <George.Dunlap@citrix.com> wrote:
>
>
> > On Dec 30, 2019, at 7:32 PM, Lars Kurth <lars.kurth@xenproject.org> wrote:
> >
> > From: Lars Kurth <lars.kurth@citrix.com>
> >
> > 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-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api

?On 15/01/2020, 10:47, "George Dunlap" <george.dunlap@citrix.com> wrote:

On 1/13/20 9:21 PM, Lars Kurth wrote:
>
>
> On 13/01/2020, 19:54, "George Dunlap" <George.Dunlap@citrix.com> wrote:
>
>
> > On Dec 30, 2019, at 7:32 PM, Lars Kurth <lars.kurth@xenproject.org> wrote:
> >
> > From: Lars Kurth <lars.kurth@citrix.com>
> >
> > 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 "<redacted>" 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 footnote "rot13 for injury/violence trigger"
‹lars_kurth› gwd, Diziet: [redacted] it is


And here is the diff

@@ -74,17 +74,20 @@ clarifications to a review or responding to questions. A simple

is normally sufficient.

-### Avoid opinion: stick to the facts
+### Avoid inflammatory and negatively charged language

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**.
+perceives the feedback. Choosing negatively charged language such as your code
+is terrible, fragile, spaghetti, inefficient, racy, messy, etc. creates a
+negative emotional response in the submitter, which can then make subsequent
+communication difficult.
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][1].
-This example illustrates the problem of using opinion in code reviews vs.
-using facts extremely well.
+This example illustrates the differences between an inflammatory and fact-based
+description extremely well.

> ?? (guo3 jiao3): foot-binding (a vile feudal practice which crippled women
> both physically and spiritually)
@@ -106,11 +109,10 @@ Compare this to the [Wikipedia entry][2]
> 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…
+> into the sole of the foot until [redacted] ...

-Without going into the details of foot-binding, it is noticeable that none of
-what is written above uses opinion which could be interpreted as inflammatory
-language. It is a list of simple facts that are laid out in a way that make it
+Without going into the details of foot-binding, it is noticeable that the
+definition is a list of simple facts that are laid out in a way that make it
obvious what the correct conclusion is.

Because the Wikipedia entry is entirely fact based it is more powerful and
@@ -120,7 +122,7 @@ Making statements in code reviews such as
> Your code is garbage
> This idea is stupid

-besides being an opinion is rude and counter productive
+besides negatively charged, rude and counter productive
* It will make the patch author angry: instead of finding a solution to the
problem the author will spend time and mental energy wrestling with their
feelings

@George, @Ian: let me know whether this is better and addresses your
concerns

Lars

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

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-api mailing list
Xen-api@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-api