Difference between revisions of "Code review"
m (→Good reads) |
(→Good reads: add Atlassian article on knowledge sharing) |
||
(10 intermediate revisions by the same user not shown) | |||
Line 1: | Line 1: | ||
This page documents code review practices used for [[Software Heritage]] development. | This page documents code review practices used for [[Software Heritage]] development. | ||
− | + | == Guidelines == | |
− | + | Please adhere to the following guidelines to perform and obtain code reviews (CRs) in the context of Software Heritage development: | |
− | # | + | # '''CRs are strongly recommended''' for any non-trivial code change, but not mandatory (nor enforced at the VCS level). |
# The CR [[Code review in Phabricator|'''workflow''']] is implemented using Phabricator/Differential. | # The CR [[Code review in Phabricator|'''workflow''']] is implemented using Phabricator/Differential. | ||
− | # Explicitly '''suggest reviewer'''(s) when submitting new CR requests: either the most knowledgeable person(s) for the target code or the general [https://forge.softwareheritage.org/project/view/50/ | + | # Explicitly '''suggest reviewer'''(s) when submitting new CR requests: either the most knowledgeable person(s) for the target code or the general [https://forge.softwareheritage.org/project/view/50/ reviewers] group (which is the [https://forge.softwareheritage.org/H18 default]). |
# '''Review anything you want''': no matter the suggested reviewer(s), feel free to review any outstanding CR. | # '''Review anything you want''': no matter the suggested reviewer(s), feel free to review any outstanding CR. | ||
# '''One LGTM is enough''': feel free to approve any outstanding CR. | # '''One LGTM is enough''': feel free to approve any outstanding CR. | ||
− | # '''Review every day''': CRs should be timely as fellow developers will wait for them. To make CRs | + | # '''Review every day''': CRs should be timely as fellow developers will wait for them. To make CRs sustainable each developer should strive to dedicate a fixed minimum amount of CR time every (work) day. |
For more detailed suggestions (and much more) on the motivational and practical aspects of code reviews see Good reads below. | For more detailed suggestions (and much more) on the motivational and practical aspects of code reviews see Good reads below. | ||
Line 19: | Line 19: | ||
* '''[https://medium.com/palantir/code-review-best-practices-19e02780015f Best practices]''' (Palantir) ← comprehensive and recommended read, especially if you're short on time | * '''[https://medium.com/palantir/code-review-best-practices-19e02780015f Best practices]''' (Palantir) ← comprehensive and recommended read, especially if you're short on time | ||
− | * [https://github.com/thoughtbot/guides/tree/master/code-review Best practices] | + | * [https://github.com/thoughtbot/guides/tree/master/code-review Best practices] (Thoughtbot) |
+ | * [https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/ Best practices] (Smart Bear) | ||
* [https://www.codeproject.com/Articles/524235/Codeplusreviewplusguidelines Review checklist] (Code Project) | * [https://www.codeproject.com/Articles/524235/Codeplusreviewplusguidelines Review checklist] (Code Project) | ||
+ | * [https://blog.codinghorror.com/code-reviews-just-do-it/ Motivation: code quality] (Coding Horror) | ||
* [https://blog.fullstory.com/what-we-learned-from-google-code-reviews-arent-just-for-catching-bugs/ Motivation: team culture] (Google & FullStory) | * [https://blog.fullstory.com/what-we-learned-from-google-code-reviews-arent-just-for-catching-bugs/ Motivation: team culture] (Google & FullStory) | ||
− | |||
* [http://www.processimpact.com/articles/humanizing_reviews.pdf Motivation: humanizing peer reviews] (Wiegers) | * [http://www.processimpact.com/articles/humanizing_reviews.pdf Motivation: humanizing peer reviews] (Wiegers) | ||
+ | * [https://www.atlassian.com/agile/software-development/code-reviews Motivation: sharing knowledge] (Atlassian) | ||
== See also == | == See also == | ||
* [[Code review in Phabricator]] | * [[Code review in Phabricator]] | ||
+ | * [[Coding guidelines]] | ||
[[Category:Software development]] | [[Category:Software development]] |
Revision as of 18:24, 12 October 2018
This page documents code review practices used for Software Heritage development.
Guidelines
Please adhere to the following guidelines to perform and obtain code reviews (CRs) in the context of Software Heritage development:
- CRs are strongly recommended for any non-trivial code change, but not mandatory (nor enforced at the VCS level).
- The CR workflow is implemented using Phabricator/Differential.
- Explicitly suggest reviewer(s) when submitting new CR requests: either the most knowledgeable person(s) for the target code or the general reviewers group (which is the default).
- Review anything you want: no matter the suggested reviewer(s), feel free to review any outstanding CR.
- One LGTM is enough: feel free to approve any outstanding CR.
- Review every day: CRs should be timely as fellow developers will wait for them. To make CRs sustainable each developer should strive to dedicate a fixed minimum amount of CR time every (work) day.
For more detailed suggestions (and much more) on the motivational and practical aspects of code reviews see Good reads below.
Good reads
Good reads on various angles of code review:
- Best practices (Palantir) ← comprehensive and recommended read, especially if you're short on time
- Best practices (Thoughtbot)
- Best practices (Smart Bear)
- Review checklist (Code Project)
- Motivation: code quality (Coding Horror)
- Motivation: team culture (Google & FullStory)
- Motivation: humanizing peer reviews (Wiegers)
- Motivation: sharing knowledge (Atlassian)