Review Policy: Difference between revisions
Jump to navigation
Jump to search
m (give sections numbers, so it's easier to refer to them) |
(Cross-link things to look out for in review.) |
||
Line 4: | Line 4: | ||
Peer review is a proven method to improve code quality and is therefore '''strongly''' encouraged. | Peer review is a proven method to improve code quality and is therefore '''strongly''' encouraged. | ||
See also [[Things To Look Out For In Reviews]]. | |||
== 1. As the Contributor == | == 1. As the Contributor == |
Revision as of 13:12, 23 May 2024
Peer review is a proven method to improve code quality and is therefore strongly encouraged.
See also Things To Look Out For In Reviews.
1. As the Contributor
- Invite relevant reviewers.
- Always invite the respective domain experts, not somebody convenient.
- If everything else fails, broadcast a review request to a relevant mailing list (as usual, make sure to give the mail a meaningful subject).
- Give reviewers ample time to respond.
- Unless everyone who can be reasonably expected to have a relevant opinion to offer has already done so, a full working day waiting time is the absolute minimum; three days are reasonable.
- In particular, give watchers (usually higher-level maintainers) enough time to voice concerns even if you did not explicitly invite them.
- This applies to every new Patch Set in turn.
- Discuss objections. Do not override a -1 unless there is a broad expert consensus that the objection is unfounded.
- Do not ignore/fight the Early Warning System. Justify each override.
- Do not approve your own changes.
- If there is no candidate for a review yet, introduce somebody to the code.
- Maintainer privilege: A maintainer may approve their own change to the code they maintain if
- at least one review (+1) from somebody else is present, and
- nobody else who could approve (+2) the change can be produced within reasonable time.
2. As the Reviewer
- Ensure compliance with the Commit Policy.
- Make sure the target branch complies with the Branch Guidelines. In case of a mismatch, ask the user to re-target the affected changes server-side.
- Never approve changes outside your expertise.
- While the Approver privilege is granted globally, you are expected to know your limits.
- If you are not certain that you know all relevant policies of a particular sub-project, it's a good sign that you should not approve.
- If you can approve only parts of the change (horizontal, e.g., build system or documentation, or vertical, e.g., only QtCore), state what you approve and give +1 for that. The most appropriate maintainer will apply a vague 1+1=2 rule for the overall approval.
- Do not approve just because it would be convenient for your colleague across the room/corridor.
- If you are not really interested in a change, say so and/or give a +1 at most.