Re: [Xen-devel] [PATCH v4 7/7] Added Resolving Disagreement
On 06.01.20 13:54, Lars Kurth wrote: On 06/01/2020, 07:25, "Jürgen Groß" wrote: >+## Issue: Small functional issues >+ >+The most common area of disagreements which happen in code reviews, are >+differing opinions on whether small functional issues in a patch series have to >+be resolved or not before the code is ready to be submitted. Such disagreements >+are typically caused by different expectations related to the level of >+perfection a patch series needs to fulfil before it can be considered ready to s/fulfil/fulfill/ >+be committed. >+ >+To explain this better, I am going to use the analogy of some building work that >+has been performed at your house. Let's say that you have a new bathroom >+installed. Before paying your builder the last instalment, you perform an s/instalment/installment/ Hi Juergen: thank you for pointing out the remaining typos. I fixed these in my local tree, with the exception of the two instances above. The two issues above come down to US vs non-US English https://grammarist.com/spelling/fulfil-fulfill/ https://grammarist.com/spelling/installment-instalment/ I didn't really review the document for consistency with respect to a particular style of English spelling. It does seem though that normally I use US spelling (e.g. minimize) mostly and of course the Contributor Covenant has been written US spelling. I don't have a strong view either way and can have a go at making it consistent (e.g. in US stylespelling) I'm not really feeling strong here, but I think consistency should be the preferred way to go. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 7/7] Added Resolving Disagreement
On 06/01/2020, 07:25, "Jürgen Groß" wrote: >+## Issue: Small functional issues >+ >+The most common area of disagreements which happen in code reviews, are >+differing opinions on whether small functional issues in a patch series have to >+be resolved or not before the code is ready to be submitted. Such disagreements >+are typically caused by different expectations related to the level of >+perfection a patch series needs to fulfil before it can be considered ready to s/fulfil/fulfill/ >+be committed. >+ >+To explain this better, I am going to use the analogy of some building work that >+has been performed at your house. Let's say that you have a new bathroom >+installed. Before paying your builder the last instalment, you perform an s/instalment/installment/ Hi Juergen: thank you for pointing out the remaining typos. I fixed these in my local tree, with the exception of the two instances above. The two issues above come down to US vs non-US English https://grammarist.com/spelling/fulfil-fulfill/ https://grammarist.com/spelling/installment-instalment/ I didn't really review the document for consistency with respect to a particular style of English spelling. It does seem though that normally I use US spelling (e.g. minimize) mostly and of course the Contributor Covenant has been written US spelling. I don't have a strong view either way and can have a go at making it consistent (e.g. in US stylespelling) Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 7/7] Added Resolving Disagreement
On 30.12.19 20:32, Lars Kurth wrote: From: Lars Kurth This guide provides Best Practice on identifying and resolving common classes of disagreement Changes since v3 * Fixed broken http link (typo) Changes since v2 (added in v2) * Fix typos * Add section: "Issue: Multiple ways to solve a problem" * Changed line wrapping to 80 characters * Replaced inline style links with reference style 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 --- resolving-disagreement.md | 188 ++ 1 file changed, 188 insertions(+) create mode 100644 resolving-disagreement.md diff --git a/resolving-disagreement.md b/resolving-disagreement.md new file mode 100644 index 000..fb3b134 --- /dev/null +++ b/resolving-disagreement.md @@ -0,0 +1,188 @@ +# Resolving Disagreement + +This guide provides Best Practice on resolving disagreement, such as +* Gracefully accept constructive criticism +* Focus on what is best for the community +* Resolve differences in opinion effectively + +## Theory: Paul Graham's hierarchy of disagreement + +Paul Graham proposed a **disagreement hierarchy** in a 2008 essay +**[How to Disagree][1]**, putting types of arguments into a seven-point +hierarchy and observing that *moving up the disagreement hierarchy makes people +less mean, and will make most of them happier*. Graham also suggested that the +hierarchy can be thought of as a pyramid, as the highest forms of disagreement +are rarer. + +| ![Graham's Hierarchy of Disagreement][2]| +| *A representation of Graham's hierarchy of disagreement from [Loudacris][3] + modified by [Rocket000][4]* | + +In the context of the Xen Project we strive to **only use the top half** of the +hierarchy. **Name-calling** and **Ad hominem** arguments are not acceptable +within the Xen Project. + +## Issue: Scope creep + +One thing which occasionally happens during code review is that a code reviewer +asks or appears to ask the author of a patch to implement additional +functionalities. + +This could take for example the form of +> Do you think it would be useful for the code to do XXX? +> I can imagine a user wanting to do YYY (and XXX would enable this) + +That potentially adds additional work for the code author, which they may not +have the time to perform. It is good practice for authors to consider such a +request in terms of +* Usefulness to the user +* Code churn, complexity or impact on other system properties +* Extra time to implement and report back to the reviewer + +If you believe that the impact/cost is too high, report back to the reviewer. +To resolve this, it is advisable to +* Report your findings +* And then check whether this was merely an interesting suggestion, or something + the reviewer feels more strongly about + +In the latter case, there are typically several common outcomes +* The **author and reviewer agree** that the suggestion should be implemented +* The **author and reviewer agree** that it may make sense to defer + implementation +* The **author and reviewer agree** that it makes no sense to implement the + suggestion + +The author of a patch would typically suggest their preferred outcome, for +example +> I am not sure it is worth to implement XXX +> Do you think this could be done as a separate patch in future? + +In cases, where no agreement can be found, the best approach would be to get an +independent opinion from another maintainer or the project's leadership team. + +## Issue: [Bikeshedding][5] + +Occasionally discussions about unimportant but easy-to-grasp issues can lead to +prolonged and unproductive discussions. The best way to approach this is to +try and **anticipate** bikeshedding and highlight it as such upfront. However, +the format of a code review does not always lend itself well to this approach, +except for highlighting it in the cover letter of a patch series. + +However, typically Bikeshedding issues are fairly easy to recognize in a code +review, as you will very quickly get different reviewers providing differing +opinions. In this case it is best for the author or a reviewer to call out the +potential bikeshedding issue using something like + +> Looks we have a bikeshedding issue here +> I think we should call a quick vote to settle the issue + +Our governance provides the mechanisms of [informal votes][6] or +[lazy voting][7] which lend themselves well to resolve such issues. + +## Issue: Small functional issues + +The most common area of disagreements which happen in code reviews, are +differing opinions on whether small functional issues in a patch series have to +be resolved or not before the code is ready to be submitted. Such disagreements +are typically caused by different expectations related to the
[Xen-devel] [PATCH v4 7/7] Added Resolving Disagreement
From: Lars Kurth This guide provides Best Practice on identifying and resolving common classes of disagreement Changes since v3 * Fixed broken http link (typo) Changes since v2 (added in v2) * Fix typos * Add section: "Issue: Multiple ways to solve a problem" * Changed line wrapping to 80 characters * Replaced inline style links with reference style 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 --- resolving-disagreement.md | 188 ++ 1 file changed, 188 insertions(+) create mode 100644 resolving-disagreement.md diff --git a/resolving-disagreement.md b/resolving-disagreement.md new file mode 100644 index 000..fb3b134 --- /dev/null +++ b/resolving-disagreement.md @@ -0,0 +1,188 @@ +# Resolving Disagreement + +This guide provides Best Practice on resolving disagreement, such as +* Gracefully accept constructive criticism +* Focus on what is best for the community +* Resolve differences in opinion effectively + +## Theory: Paul Graham's hierarchy of disagreement + +Paul Graham proposed a **disagreement hierarchy** in a 2008 essay +**[How to Disagree][1]**, putting types of arguments into a seven-point +hierarchy and observing that *moving up the disagreement hierarchy makes people +less mean, and will make most of them happier*. Graham also suggested that the +hierarchy can be thought of as a pyramid, as the highest forms of disagreement +are rarer. + +| ![Graham's Hierarchy of Disagreement][2]| +| *A representation of Graham's hierarchy of disagreement from [Loudacris][3] + modified by [Rocket000][4]* | + +In the context of the Xen Project we strive to **only use the top half** of the +hierarchy. **Name-calling** and **Ad hominem** arguments are not acceptable +within the Xen Project. + +## Issue: Scope creep + +One thing which occasionally happens during code review is that a code reviewer +asks or appears to ask the author of a patch to implement additional +functionalities. + +This could take for example the form of +> Do you think it would be useful for the code to do XXX? +> I can imagine a user wanting to do YYY (and XXX would enable this) + +That potentially adds additional work for the code author, which they may not +have the time to perform. It is good practice for authors to consider such a +request in terms of +* Usefulness to the user +* Code churn, complexity or impact on other system properties +* Extra time to implement and report back to the reviewer + +If you believe that the impact/cost is too high, report back to the reviewer. +To resolve this, it is advisable to +* Report your findings +* And then check whether this was merely an interesting suggestion, or something + the reviewer feels more strongly about + +In the latter case, there are typically several common outcomes +* The **author and reviewer agree** that the suggestion should be implemented +* The **author and reviewer agree** that it may make sense to defer + implementation +* The **author and reviewer agree** that it makes no sense to implement the + suggestion + +The author of a patch would typically suggest their preferred outcome, for +example +> I am not sure it is worth to implement XXX +> Do you think this could be done as a separate patch in future? + +In cases, where no agreement can be found, the best approach would be to get an +independent opinion from another maintainer or the project's leadership team. + +## Issue: [Bikeshedding][5] + +Occasionally discussions about unimportant but easy-to-grasp issues can lead to +prolonged and unproductive discussions. The best way to approach this is to +try and **anticipate** bikeshedding and highlight it as such upfront. However, +the format of a code review does not always lend itself well to this approach, +except for highlighting it in the cover letter of a patch series. + +However, typically Bikeshedding issues are fairly easy to recognize in a code +review, as you will very quickly get different reviewers providing differing +opinions. In this case it is best for the author or a reviewer to call out the +potential bikeshedding issue using something like + +> Looks we have a bikeshedding issue here +> I think we should call a quick vote to settle the issue + +Our governance provides the mechanisms of [informal votes][6] or +[lazy voting][7] which lend themselves well to resolve such issues. + +## Issue: Small functional issues + +The most common area of disagreements which happen in code reviews, are +differing opinions on whether small functional issues in a patch series have to +be resolved or not before the code is ready to be submitted. Such disagreements +are typically caused by different expectations related to the level of +perfection a patch series needs
