Difference between revisions of "Code review"

Revision as of 12:55, 12 October 2018

This page documents code review practices used for Software Heritage development.

WORK IN PROGRESS page

Code reviews (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.
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 team-sustainable each developer should allocate a fixed minimum amount of time for doing CR 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 on various angles of code review:

Best practices (Palantir) ← comprehensive and recommended read, especially if you're short on time
Best practices (Thoughtbot)
Review checklist (Code Project)
Motivation: team culture (Google & FullStory)
Motivation: code quality (Coding Horror)
Motivation: humanizing peer reviews (Wiegers)

@@ Line 5: / Line 5: @@
 == Guidelines ==
-# Code reviews (CRs) are '''strongly recommended''' for any code change, but not mandatory (nor enforced at the VCS level).
+# Code reviews (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.
 # 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.