From: George Dunlap Date: Thu, 10 Sep 2020 16:09:37 +0000 (+0100) Subject: Convert resolving-disagreement.md to rst X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=e3fe511153bd808a2c19d67ef57e2b5bb73f8604;p=governance.git Convert resolving-disagreement.md to rst communication-practice.rst had an incorrect link; it was listed as being in resolving-disagreements.md, but actually it was in code-review-guide. Convert this to a normal cross-reference. Convert titles / sections, lists, quotes, doc references and so on as before. Convert figure as appropriate. Signed-off-by: George Dunlap --- diff --git a/source/code-review-guide.rst b/source/code-review-guide.rst index 9c0040e..5a046a2 100644 --- a/source/code-review-guide.rst +++ b/source/code-review-guide.rst @@ -276,6 +276,7 @@ thread onto a code-base. Different people use different techniques from using addressed * A combination of the above +.. _code-review-problematic-patch-review: .. _problems: Problematic Patch Reviews diff --git a/source/communication-practice.rst b/source/communication-practice.rst index 3ccc087..70f5b8c 100644 --- a/source/communication-practice.rst +++ b/source/communication-practice.rst @@ -385,7 +385,8 @@ A pragmatic approach would be to style and minor issues in the review, until the significant flaw is addressed This saves both the patch author and reviewer(s) time. Note that some -background is covered in detail in `Problematic Patch Reviews <4_>`_. +background is covered in detail in +:ref:`Problematic Patch Reviews `. Reviewers: Welcome newcomers @@ -457,6 +458,8 @@ This section contains common communication issues and provides suggestions on how to avoid them and resolve them. These are **general** issues which affect **all** online communication. As such, we can only try and do our best. +.. _communication-practice-misunderstandings: + Misunderstandings ----------------- @@ -569,7 +572,6 @@ how to avoid and resolve issues. .. _1: https://youtu.be/ehZvBmrLRwg?t=834 .. _2: https://en.wikipedia.org/wiki/Foot_binding .. _3: https://xenproject.org/developers/governance/#expressingopinion -.. _4: resolving-disagreement.md#problems .. _5: https://www.wired.com/2006/02/the-secret-cause-of-flame-wars/ .. _6: https://xenproject.org/help/irc/ .. _8: https://en.wikipedia.org/wiki/Erin_Meyer diff --git a/source/index.rst b/source/index.rst index be162cf..c8b45d8 100644 --- a/source/index.rst +++ b/source/index.rst @@ -14,6 +14,7 @@ Welcome to XenProject Governance's documentation! communication-guide code-review-guide communication-practice + resolving-disagreement Indices and tables ================== diff --git a/source/resolving-disagreement.md b/source/resolving-disagreement.md deleted file mode 100644 index a635352..0000000 --- a/source/resolving-disagreement.md +++ /dev/null @@ -1,188 +0,0 @@ -# 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 to fulfill before it can be considered ready to -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 installment, you perform an -inspection and you find issues such as -* The seals around the bathtub are not perfectly even -* When you open the tap, the plumbing initially makes some loud noise -* The shower mixer has been installed the wrong way around - -In all these cases, the bathroom is perfectly functional, but not perfect. At -this point you have the choice to try and get all the issues addressed, which in -the example of the shower mixer may require significant re-work and potentially -push-back from your builder. You may have to refer to the initial statement of -work, but it turns out it does not contain sufficient information to ascertain -whether your builder had committed to the level of quality you were expecting. - -Similar situations happen in code reviews very frequently and can lead to a long -discussion before it can be resolved. The most important thing is to -**identify** a disagreement as such early and then call it out. Tips on how to -do this, can be found [here][8]. - -At this point, you will understand why you have the disagreement, but not -necessarily agreement on how to move forward. An easy fix would be to agree to -submit the change as it is and fix it in future. In a corporate software -engineering environment this is the most likely outcome, but in open source -communities additional concerns have to be considered. -* Code reviewers frequently have been in this situation before with the most - common outcome that the issue is then never fixed. By accepting the change, - the reviewers have no leverage to fix the issue and may have to spend effort - fixing the issue themselves in future as it may impact the product they built - on top of the code. -* Conversely, a reviewer may be asking the author to make too many changes of - this type which ultimately may lead the author to not contribute to the - project again. -* An author, which consistently does not address **any** of these issues may - end up getting a bad reputation and may find future code reviews more - difficult. -* An author which always addresses **all** of these issues may end up getting - into difficulties with their employer, as they are too slow getting code - upstreamed. - -None of these outcomes are good, so ultimately a balance has to be found. At -the end of the day, the solution should focus on what is best for the community, -which may mean asking for an independent opinion as outlined in the next -section. - -## Issue: Multiple ways to solve a problem - -Frequently it is possible that a problem can be solved in multiple ways and it -is not always obvious which one is best. Code reviewers tend to follow their -personal coding style when reviewing code and sometimes will suggest that a -code author makes changes to follow their own style, even when the author's -code is correct. In such cases, it is easy to disagree and start arguing. - -We recommend that the code author tries to follow the code reviewers requests, -even if they could be considered style issues, trusting the experience of the -code reviewer. Similarly, we ask code reviewers to let the contributor have the -freedom of implementation choices, where they do not have a downside. - -We do not always succeed in this, as such it is important to **identify** such a -situation and then call it out as outlined [here][8]. - -## Resolution: Asking for an independent opinion - -Most disagreements can be settled by -* Asking another maintainer or committer to provide an independent opinion on - the specific issue in public to help resolve it -* Failing this an issue can be escalated to the project leadership team, which - is expected to act as referee and make a decision on behalf of the community - -If you feel uncomfortable with this approach, you may also contact -mediation@xenproject.org to get advice. See our [Communication Guide][9] -for more information. - -## Decision making and conflict resolution in our governance - -Our [governance][A] contains several proven mechanisms to help with decision -making and conflict resolution. - -See -* [Expressing agreement and disagreement][B] -* [Lazy consensus / Lazy voting][7] -* [Informal votes or surveys][6] -* [Leadership team decisions][C] -* [Conflict resolution][D] - -[1]: http://www.paulgraham.com/disagree.html -[2]: https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg -[3]: https://www.createdebate.com/user/viewprofile/Loudacris -[4]: https://en.wikipedia.org/wiki/User:Rocket000 -[5]: https://en.wiktionary.org/wiki/bikeshedding -[6]: https://xenproject.org/developers/governance/#informal-votes-or-surveys -[7]: https://xenproject.org/developers/governance/#lazyconsensus -[8]: communication-practice.md#Misunderstandings -[9]: communication-guide.md -[A]: https://xenproject.org/developers/governance/#decisions -[B]: https://xenproject.org/developers/governance/#expressingopinion -[C]: https://xenproject.org/developers/governance/#leadership -[D]: https://xenproject.org/developers/governance/#conflict diff --git a/source/resolving-disagreement.rst b/source/resolving-disagreement.rst new file mode 100644 index 0000000..28071cf --- /dev/null +++ b/source/resolving-disagreement.rst @@ -0,0 +1,209 @@ +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. + +.. figure:: https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg + :alt: Graham's Hierarchy of Disagreement + + 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 to fulfill before it can be considered ready to +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 installment, you perform an +inspection and you find issues such as + +* The seals around the bathtub are not perfectly even +* When you open the tap, the plumbing initially makes some loud noise +* The shower mixer has been installed the wrong way around + +In all these cases, the bathroom is perfectly functional, but not perfect. At +this point you have the choice to try and get all the issues addressed, which in +the example of the shower mixer may require significant re-work and potentially +push-back from your builder. You may have to refer to the initial statement of +work, but it turns out it does not contain sufficient information to ascertain +whether your builder had committed to the level of quality you were expecting. + +Similar situations happen in code reviews very frequently and can lead to a long +discussion before it can be resolved. The most important thing is to +**identify** a disagreement as such early and then call it out. Tips on how to +do this, can be found :doc:`here `. + +At this point, you will understand why you have the disagreement, but not +necessarily agreement on how to move forward. An easy fix would be to agree to +submit the change as it is and fix it in future. In a corporate software +engineering environment this is the most likely outcome, but in open source +communities additional concerns have to be considered. + +* Code reviewers frequently have been in this situation before with the most + common outcome that the issue is then never fixed. By accepting the change, + the reviewers have no leverage to fix the issue and may have to spend effort + fixing the issue themselves in future as it may impact the product they built + on top of the code. +* Conversely, a reviewer may be asking the author to make too many changes of + this type which ultimately may lead the author to not contribute to the + project again. +* An author, which consistently does not address **any** of these issues may + end up getting a bad reputation and may find future code reviews more + difficult. +* An author which always addresses **all** of these issues may end up getting + into difficulties with their employer, as they are too slow getting code + upstreamed. + +None of these outcomes are good, so ultimately a balance has to be found. At +the end of the day, the solution should focus on what is best for the community, +which may mean asking for an independent opinion as outlined in the next +section. + +Issue: Multiple ways to solve a problem +======================================= + +Frequently it is possible that a problem can be solved in multiple ways and it +is not always obvious which one is best. Code reviewers tend to follow their +personal coding style when reviewing code and sometimes will suggest that a +code author makes changes to follow their own style, even when the author's +code is correct. In such cases, it is easy to disagree and start arguing. + +We recommend that the code author tries to follow the code reviewers requests, +even if they could be considered style issues, trusting the experience of the +code reviewer. Similarly, we ask code reviewers to let the contributor have the +freedom of implementation choices, where they do not have a downside. + +We do not always succeed in this, as such it is important to +**identify** such a situation and then call it out as outlined +:ref:`here `. + +Resolution: Asking for an independent opinion +============================================= + +Most disagreements can be settled by + +* Asking another maintainer or committer to provide an independent opinion on + the specific issue in public to help resolve it +* Failing this an issue can be escalated to the project leadership team, which + is expected to act as referee and make a decision on behalf of the community + +If you feel uncomfortable with this approach, you may also contact +mediation@xenproject.org to get advice. See our :doc:`Communication Guide ` +for more information. + +Decision making and conflict resolution in our governance +========================================================= + +Our `governance `_ contains several proven mechanisms to help with decision +making and conflict resolution. + +See + +* `Expressing agreement and disagreement `_ +* `Lazy consensus / Lazy voting <7_>`_ +* `Informal votes or surveys <6_>`_ +* `Leadership team decisions `_ +* `Conflict resolution `_ + +.. _1: http://www.paulgraham.com/disagree.html +.. _3: https://www.createdebate.com/user/viewprofile/Loudacris +.. _4: https://en.wikipedia.org/wiki/User:Rocket000 +.. _5: https://en.wiktionary.org/wiki/bikeshedding +.. _6: https://xenproject.org/developers/governance/#informal-votes-or-surveys +.. _7: https://xenproject.org/developers/governance/#lazyconsensus +.. _A: https://xenproject.org/developers/governance/#decisions +.. _B: https://xenproject.org/developers/governance/#expressingopinion +.. _C: https://xenproject.org/developers/governance/#leadership +.. _D: https://xenproject.org/developers/governance/#conflict