[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




On 27/11/2019, 18:57, "Stefano Stabellini" <sstabellini@xxxxxxxxxx> wrote:

    On Thu, 26 Sep 2019, Lars Kurth wrote:
    > From: Lars Kurth <lars.kurth@xxxxxxxxxx>
    > 
    > 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 <lars.kurth@xxxxxxxxxx>
    > ---
    > Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx
    > Cc: xen-api@xxxxxxxxxxxxxxxxxxxx
    > Cc: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
    > Cc: mirageos-devel@xxxxxxxxxxxxxxxxxxxx
    > Cc: committers@xxxxxxxxxxxxxx
    > ---
    >  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 0000000..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**.  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 opinion in code reviews vs. using facts 
extremely well.
    > +
    > +> 裹脚 (guo3 jiao3): foot-binding (a vile feudal practice which crippled 
women both
    > +> physically and spiritually)
    > +
    > +This is not something one is used to hearing from dictionary entries. 
Once you
    > +investigate the practice foot-binding, it is hard to disagree with the 
dictionart entry.
    > +However, the statement does not contain much information. If you read it 
without
    > +knowing what foot-binding is, it is hard to be convinced by this 
statement. The main
    > +take-away is that the author of the dictionary entry had strong opinions 
about this topic.
    > +It does not tell you, why you should have the same opinion.
                           ^ remove ,
    
    > +
    > +Compare this to the (Wikipedia 
entry)[https://en.wikipedia.org/wiki/Foot_binding]
    > +
    > +> 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…
    > +
    > +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 obvious what the 
correct conclusion
    > +is.
    > +
    > +Because the Wikipedia entry is entirely fact based it is more powerful 
and persuasive
    > +then the dictionary entry. The same applies to code reviews.
    > +
    > +Making statements in code reviews such as
    > +> Your code is garbage
    > +> This idea is stupid
    > +
    > +besides being an opinion is 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
    > +* It does not contain any information
    > +* Facts are both more powerful and more persuasive
    > +
    > +Consider the following two pieces of feedback on a piece of code
    > +> This piece of code is confusing
    > +> It took me a long time to figure out what was going on here
    > +
    > +The first example expresses an opinion, whereas the second re-phrases 
the statement
    > +in terms of what you experienced, which is a fact.
    > +
    > +Other examples:
    > +> BAD: This is fragile
    > +> SOMEWHAT BETTER: This seems fragile to me
    > +> BEST: If X happens, Y will happen.
    > +
    > +A certain piece of code can be written in many different ways: this can 
lead to
    > +disagreements on the best architecture, design or coding pattern. As 
already pointed out
    > +in this section: avoid feedback that is opinion-based and thus does not 
add any value.
    > +Back your criticism (or idea on how to solve a problem) with a sensible 
rationale.
    > +
    > +### Review the code, not the person
    > +Without realizing it, it is easy to overlook the difference between 
insightful critique of
    > +code and personal criticism. Let's look at a theoretical function where 
there is an
    > +opportunity to return out of the function early. In this case, you could 
say
    > +
    > +> You should return from this function early, because of XXX
    > +
    > +On its own, there is nothing wrong with this statement. However, a code 
review is made
    > +up of multiple comments and using **You should** consistently can start 
to feel negative
    > +and can be mis-interpreted as a personal attack. Using something like 
avoids this issue:
    > +
    > +> Returning from this function early is better, because of XXX
    > +
    > +Without personal reference, a code review will communicate the problem, 
idea or issue
    > +without risking mis-interpretation.
    > +
    > +### 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.
    > +
    > +If you tend to use a terse commenting style and you do not know whether 
the author
    > +is OK with it, it is often a good idea to compensate for it in the code 
review opening
    > +(where you express appreciation) or when there is a need for verbose 
expression.
    > +
    > +It is also entirely fine to mention that you have a fairly terse 
communication style
    > +and ask whether the author is OK with it. In almost all cases, they will 
be: by asking
    > +you are showing empathy that helps counteract a negative impression.
    > +
    > +### Code Review Comments should be actionable
    > +Code review comments should be actionable: in other words, it needs to 
be clear
    > +what the author of the code needs to do to address the issue you 
identified.
    > +
    > +Statements such as
    > +> BAD: This is wrong
    > +> BAD: This does not work
    > +> BETTER, BUT NOT GOOD: This does not work, because of XXX
    > +
    > +do not normally provide the author of a patch with enough information to 
send out a
    > +new patch version. By doing this, you essentially force the patch author 
to **find** and
    > +**implement** an alternative, which then may also not be acceptable to 
you as the
    > +**reviewer** of the patch.
    > +
    > +A better way to approach this is to say
    > +
    > +> This does not work, because of XXX
    > +> You may want to investigate YYY and ZZZ as alternatives
    > +
    > +In some cases, it may not be clear whether YYY or ZZZ are the better 
solution. As a
    > +reviewer you should be as up-front and possible in such a case and say 
something like
    > +
    > +> I am not sure whether YYY and ZZZ are better, so you may want to 
outline your
    > +> thoughts about both solutions by e-mail first, such that we can decide 
what works
    > +> best
    > +
    > +### Identify the severity of an issue or disagreement
    > +By default, every comment which is made **ought to be addressed** by the 
author.
    > +However, often reviewers note issues, which would be nice if they were 
addressed,
    > +but are not mandatory.
    > +
    > +Typically, reviewers use terminology such as
    > +> This would be a nice-to-have
    > +> This is not a blocker
    > +
    > +Some maintainers use
    > +> NIT: XXX
    > +
    > +however, it is sometimes also used to indicate a minor issue that 
**must** be fixed.
    >
    > +During a code review, it can happen that reviewer and author disagree on 
how to move
    > +forward. The default position when it comes to disagreements is that 
**both parties
    > +want to argue their case**. However, frequently one or both parties do 
not feel that
    > +strongly about a specific issue.
    > +
    > +Within the Xen Project, we have [a 
way](https://xenproject.org/developers/governance/#expressingopinion)
    > +to highlight one's position on proposals, formal or informal votes using 
the following
    > +notation:
    > +> +2 : I am happy with this proposal, and I will argue for it
    > +> +1 : I am happy with this proposal, but will not argue for it
    > +> 0 : I have no opinion
    > +> -1 : I am not happy with this proposal, but will not argue against it
    > +> -2 : I am not happy with this proposal, and I will argue against it
    > +
    > +You can use a phrase such as
    > +> I am not happy with this suggestion, but will not argue against it
    > +
    > +to make clear where you stand, while recording your position. 
Conversely, a reviewer
    > +may do something similar
    > +> I am not happy with XYZ, but will not argue against it [anymore]
    > +> What we have now is good enough, but could be better
    
    It is not just about the willingness of somebody to argue a point, which
    is the important thing when voting. During code reviews it is perfectly
    fine to make suggestions which are just optional for multiple reasons,
    including that they might be too taxing for the contributor.
    
    So, I think we should add that it would be best to use words that make it
    clear whether something is optional or whether it is required, see my
    reply to patch #6, I wrote an example there.
    
    
    
    > +### Authors: responding to review comments
    > +Typically patch authors are expected to **address all** review comments 
in the next
    > +version of a patch or patch series. In a smooth-running code review 
where you do not
    > +have further questions it is not at all necessary to acknowledge the 
changes you are
    > +going to make:
    > +* Simply send the next version with the changes addressed and record it 
in the
    > +change-log
    > +
    > +When there is discussion, the normal practice is to remove the portion 
of the e-mail
    > +thread where there is agreement. Otherwise, the thread can become 
exceptionally
    > +long.
    > +
    > +In cases where there was discussion and maybe disagreement, it does 
however make
    > +sense to close the discussion by saying something like
    > +
    > +> ACK
    > +> Seems we are agreed, I am going to do this
    > +
    > +Other situations when you may want to do this are cases where the 
reviewer made
    > +optional suggestions, to make clear whether the suggestion will be 
followed or
    > +not.
    > +
    > +### Avoid uncommon words: not everyone is a native English speaker
    > +Avoid uncommon words both when reviewing code or responding to a review. 
Not
    > +everyone is a native English speaker. The use of such words can come 
across badly and
    > +can lead to misunderstandings.
    > +
    > +### Prioritize significant flaws
    > +If a patch or patch series has significant flaws, such as
    > +* It is built on wrong assumptions
    > +* There are issues with the architecture or the design
    > +
    > +it does not make sense to do a detailed code review. In such cases, it 
is best to
    > +focus on the major issues first and deal with style and minor issues in 
a subsequent
    > +review. This reduces the workload on both the reviewer and patch author. 
However,
    > +reviewers should make clear that they have omitted detailed review 
comments and
    > +that these will come later.
    
    Maybe we want to expand on this a bit. Not all series are based on
    flawed assumptions, but all series have different class of changes that
    are required for acceptance, from major code modifications to minor code
    style fixes.
    
    I think we should say that it is good practice to ask for any major
    changes early on, during the first or second iteration of the series.
    It would be best to avoid asking for major changes at v9 if possible.
    
    
    Something else which is missing in this document, and it is purely for
    reviewers, is to be careful doing reviews late in the cycle when another
    maintainer/reviewer has already provided feedback on the series multiple
    times previously. For instance, if reviewer R1 has been doing reviews
    from the first version of the series and contributor C has been
    addressing all comments, it would be best if reviewer R2 didn't come in
    providing detailed feedback months later at v5, unless their requests
    are actually strictly necessary (i.e. they spotted a bug). The main
    reason is that it is difficult not to let your own personal style (code
    style, the way to lay out the code) sip through review comments, and it
    can cause double-effort for the author if he/she already made changes
    according R1's personal style. However, in general, it would be best to
    limit "personal style" requests for changes anyway, see my comment to
    patch #6.
      
I see whether I can add something. I do like Rich's suggestion to use the Shift 
Left" terminology (https://devopedia.org/shift-left) of which this is kind of 
an instance. The same applies 

I can put together a v2, with typos addressed and expand some sections.

Regards
Lars


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.