From fdb78c28bca31aabffd7700ee9a78bfd79a79ccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Sat, 11 Jan 2020 00:08:19 -0800 Subject: [PATCH 1/4] Add an initial guide on review culture Lately we've been discussing how cultivating [review culture][1] across SourceCred will help us stay aligned as we grow our community. In particular, I think we should start producing shared "docs artifacts" that we've reviewed and we stand behind as a community. By "docs artifacts" I mean both things like user guides, and also community touchstones. So, to kick things off, I wrote this guide on review culture, which I hope will be one of the first docs artifacts. Please review it and let me know what you think. :) [1]: https://discourse.sourcecred.io/t/spontanious-community-call/543 --- culture/review_culture.md | 142 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 culture/review_culture.md diff --git a/culture/review_culture.md b/culture/review_culture.md new file mode 100644 index 00000000..3abb8e37 --- /dev/null +++ b/culture/review_culture.md @@ -0,0 +1,142 @@ +> If you want to go fast, go alone. If you want to go far, go together. + +As a community, we value going far over going fast. To go far, we need to stay +aligned in our values, goals, and practices. One of the ways that we do this is +through cultivating review culture. + +# On Review Culture + +Review culture came to us from the practices of software engineers. Large +codebases are incredibly complex: they contain the ideas and work of hundreds +of people, manifested in millions of lines of code. They need to remain +logically consistent, even as they grow and change. A big part of how engineers +manage is through code review: every single change to the system is reviewed by +at least one other person before becoming official, or "merging". + +Ensuring that all work is reviewed brings several benefits. Most obviously, it +leads to higher quality work, because the final output includes feedback and +suggestions from others. Just as importantly, reviews enable the exchange of +knowledge, context, and perspective within the team. In a good review, both the +contributor and the reviewer can teach each other about how work gets done on +the project. Finally, review ensures that every part of the project is +understood by at least two people. + +# The Review Process + +## Changes +The review process orients around _changes_, which are the units of work being +reviewed. + +A change can take many forms, for example: +- Adding a new guide or piece of documentation +- Adding a new module to the codebase +- Modifying an existing guide or module + + +## Participants in the Review Process + +### Contributor + +The contributor is the person proposing a change. Anyone can decide to be a +contributor: the only requirements are that you have a change you need to +propose, and that you agree to be a respectful participant in the review +process. (When we have a code of conduct--link to it here.) + +### Reviewer + +The reviewers are the people who review the change, suggest changes to it, and +eventually approve it. Anyone in the community can act as a reviewer: the only +requirements are that you have some insight to share (or questions to ask) +about the change, and that you agree to be a respectful participant in the +process. + +### Maintainer + +The maintainers are responsible for the health and maintenance of the project, +and have final say in the review process. Maintainers can block changes they +think are harmful, or approve a change over others' objections. Someone can be +a maintainer within a specific domain: for example, acting as maintainer for a +particular repository, or even maintaining a particular guide. Since +maintainers have special powers, they also have a special responsibility to act +thoughtfully and fairly. New maintainers are chosen by the existing +maintainers. In general, people who have a long track record of acting as +contributors and reviewers may be good candidates for becoming maintainers. + +## Lifecycle of a Review + +### Proposal +The first step is for a contributor to propose a _change_. +The contributor needs to do two things at this stage: +- Flesh out the change (e.g. writing the code), so that there is something to review +- Explain the change (e.g. writing a pull request description), so that reviewers can easily understand it + +Both steps are vital, and contributors should put real effort into explaining the change well. +Its not fair to ask a reviewer to puzzle through an undocumented change. + +Finally, before requesting a review, the contributor should _self-review_ their change. +It's amazing how many issues can be spotted and fixed during self-review. +This is respectful of reviewers' time, and makes the process a lot smoother. + +### Review + +Once a change is ready and has been self-reviewed, it's ready for review. Since +most SourceCred artifacts are tracked via repositories on GitHub, this will +usually mean creating a pull request. + +Once the change is proposed, anyone in the project can add their own review. +The person proposing the change can also request specific reviewers, if certain +individiuals have context or knowledge thet will make their reivew particularly +valuable. + +On finishing their review, the reviewer can: +- ask questions to better understand the change +- suggest changes to it +- approve the change + +Most changes go through a few rounds of reviews before being approved. + +### Approval and Merging + +During the review process, a successful change will accumulate "approvals" from +satisfied reviewers. At some point, the change will have "enough approvals" and +will be ready to merge. There's no hard-and-fast rule to determine when a +change has enough approvals. Generally, simple and small changes are easy to +approve, whereas changes that are breaking new ground, or have wide-ranging +consequences, will require more reviews. If you're unsure as to whether a +change is ready to merge, ask one of the maintainers. + +### Changes that never merge + +Some changes don't ever merge. A few good reasons why this may happen include: +- Reviewers may find that the change isn't actually needed, is basically + unsound, or isn't aligned with the project's values or goals +- Over the course of review, the contributor or reviewers may find a different + and better way to achieve the same goals + +# Best Practices for Reviewers + +## Be respectful and appreciative + +As a reviewer, you should be respectful and appreciative towards the person +proposing the change. This is true especially when you quite disagree with the +change itself. It's fine to express strong beliefs that a change is misguided, +but you shouldn't make the contributor feel belittled. + +## Limit nit-picking + +Nit-picking is when a review focuses more on small, superficial details (e.g. +precise formatting or word choice) rather than on the substance of the change. +Nit-picking is a lot easier than substantive review, which can make it +tempting. It's ok to nit pick in moderation; when nit-picking, it's polite to +preface your suggestion with "nit", as in: "nit: this comma should actually be +a semicolon". Avoid holding up a review over nits. + +## Avoid "not how I would have done it" + +As a reviewer, you'll often notice that someone has made a change differently +from how you would have made it. It can be tempting to think that your way is +_the_ right way, and to ask the contributor to change their approach. Before +doing this, you should ask: in what ways is doing it my way materially better? +What benefits come from the changed approach? If you don't have clear answers +to these questions, don't ask for the change. + From b5ce2cec1b56d81b974647ff1feb86ee06e6dc9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Mon, 13 Jan 2020 20:55:48 -0800 Subject: [PATCH 2/4] Address review feedback --- culture/review_culture.md | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/culture/review_culture.md b/culture/review_culture.md index 3abb8e37..d9fd04d3 100644 --- a/culture/review_culture.md +++ b/culture/review_culture.md @@ -52,15 +52,14 @@ process. ### Maintainer -The maintainers are responsible for the health and maintenance of the project, -and have final say in the review process. Maintainers can block changes they -think are harmful, or approve a change over others' objections. Someone can be -a maintainer within a specific domain: for example, acting as maintainer for a -particular repository, or even maintaining a particular guide. Since -maintainers have special powers, they also have a special responsibility to act -thoughtfully and fairly. New maintainers are chosen by the existing -maintainers. In general, people who have a long track record of acting as -contributors and reviewers may be good candidates for becoming maintainers. +Maintainers are responsible for the health and maintenance of a portion of the +project. They are the stewards of the review process, and have the privileges +needed to move forward or block a change. Not everyone can be a maintainer; it +requires deep knowledge of the project, high trust from the community, +experience as both a contributor and reviewer, and substantial commitment. +Since we invest maintainers with special privileges and trust, we have high +expectations of them. Those acting as maintainers must be respectful, +considerate, and judicious in the way they exercise their powers. ## Lifecycle of a Review @@ -71,7 +70,8 @@ The contributor needs to do two things at this stage: - Explain the change (e.g. writing a pull request description), so that reviewers can easily understand it Both steps are vital, and contributors should put real effort into explaining the change well. -Its not fair to ask a reviewer to puzzle through an undocumented change. +Doing so is respectful of reviewers' time, and makes it easier for others in the community to follow +along with the project and get involved themselves. Finally, before requesting a review, the contributor should _self-review_ their change. It's amazing how many issues can be spotted and fixed during self-review. @@ -89,11 +89,11 @@ individiuals have context or knowledge thet will make their reivew particularly valuable. On finishing their review, the reviewer can: -- ask questions to better understand the change -- suggest changes to it -- approve the change +- Ask questions to better understand the change +- Suggest changes to it +- Approve the change -Most changes go through a few rounds of reviews before being approved. +Many changes go through a few rounds of reviews before being approved. ### Approval and Merging From 4b7ee6f0779d903b1883f39da5ce6e058f511d77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Sun, 19 Jan 2020 16:20:28 -0800 Subject: [PATCH 3/4] Address review feedback -- round 2 --- culture/review_culture.md | 47 ++++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/culture/review_culture.md b/culture/review_culture.md index d9fd04d3..a637601e 100644 --- a/culture/review_culture.md +++ b/culture/review_culture.md @@ -28,27 +28,25 @@ The review process orients around _changes_, which are the units of work being reviewed. A change can take many forms, for example: -- Adding a new guide or piece of documentation -- Adding a new module to the codebase -- Modifying an existing guide or module - +- Adding or changing official SourceCred documentation (in [sourcecred/docs](https://github.com/sourcecred/docs)) +- Adding or changing the SourceCred codebase +- Proposing a re-organization of the [SourceCred Discourse](https://discourse.sourcecred.io/) ## Participants in the Review Process ### Contributor The contributor is the person proposing a change. Anyone can decide to be a -contributor: the only requirements are that you have a change you need to -propose, and that you agree to be a respectful participant in the review -process. (When we have a code of conduct--link to it here.) +contributor: the only requirements are that they have a change to propose, and +that they agree to be a respectful participant in the community. ### Reviewer The reviewers are the people who review the change, suggest changes to it, and eventually approve it. Anyone in the community can act as a reviewer: the only -requirements are that you have some insight to share (or questions to ask) -about the change, and that you agree to be a respectful participant in the -process. +requirements are that they have some insight to share (or questions to ask) +about the change, and that they agree to be a respectful participant in the +community. ### Maintainer @@ -64,18 +62,26 @@ considerate, and judicious in the way they exercise their powers. ## Lifecycle of a Review ### Proposal -The first step is for a contributor to propose a _change_. -The contributor needs to do two things at this stage: +The first step is for a contributor to come up with and propose a _change_. +Before starting work on the change, they may want to run it by the community. +Often times community members will have great feedback on how to approach the +change, and whether it's valuable. + +Once the contributor wants to move forward with the change, they will formally +propose it. They need to do two things at this stage: - Flesh out the change (e.g. writing the code), so that there is something to review - Explain the change (e.g. writing a pull request description), so that reviewers can easily understand it -Both steps are vital, and contributors should put real effort into explaining the change well. -Doing so is respectful of reviewers' time, and makes it easier for others in the community to follow -along with the project and get involved themselves. +Both steps are vital, and contributors should put real effort into explaining +the change well. Doing so is respectful of reviewers' time, and makes it easier +for others in the community to follow along with the project and get involved +themselves. Finally, the change's explanation acts as documentation for future +contributors hoping to understand the project. -Finally, before requesting a review, the contributor should _self-review_ their change. -It's amazing how many issues can be spotted and fixed during self-review. -This is respectful of reviewers' time, and makes the process a lot smoother. +Finally, before requesting a review, the contributor should _self-review_ their +change. It's amazing how many issues can be spotted and fixed during +self-review. This is respectful of reviewers' time, and makes the process a lot +smoother. ### Review @@ -92,6 +98,7 @@ On finishing their review, the reviewer can: - Ask questions to better understand the change - Suggest changes to it - Approve the change +- Suggest closing the change as 'wontfix' Many changes go through a few rounds of reviews before being approved. @@ -102,8 +109,8 @@ satisfied reviewers. At some point, the change will have "enough approvals" and will be ready to merge. There's no hard-and-fast rule to determine when a change has enough approvals. Generally, simple and small changes are easy to approve, whereas changes that are breaking new ground, or have wide-ranging -consequences, will require more reviews. If you're unsure as to whether a -change is ready to merge, ask one of the maintainers. +consequences, will require more reviews. If contributors are unsure as to +whether a change is ready to merge, they should ask a maintainer. ### Changes that never merge From 724e0caa5194fe9e07c93f1139ec33cc8db71458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Sun, 19 Jan 2020 18:56:42 -0800 Subject: [PATCH 4/4] Address minor nit --- culture/review_culture.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/culture/review_culture.md b/culture/review_culture.md index a637601e..5f18d6a8 100644 --- a/culture/review_culture.md +++ b/culture/review_culture.md @@ -29,7 +29,7 @@ reviewed. A change can take many forms, for example: - Adding or changing official SourceCred documentation (in [sourcecred/docs](https://github.com/sourcecred/docs)) -- Adding or changing the SourceCred codebase +- Adding to or changing the SourceCred codebase - Proposing a re-organization of the [SourceCred Discourse](https://discourse.sourcecred.io/) ## Participants in the Review Process