Review Policy: Difference between revisions
Jump to navigation
Jump to search
(long live the review policy!) |
m (too much z (for the time being ^^)) |
||
Line 23: | Line 23: | ||
# Ensure compliance with the [[Commit Policy]]. | # Ensure compliance with the [[Commit Policy]]. | ||
# Make sure the target branch complies with the [[Branch Guidelines]]. In case of a mismatch, ask a [https://codereview.qt-project.org/#/admin/groups/1,members Gerrit Administrator] to re-target the affected changes server-side. | # Make sure the target branch complies with the [[Branch Guidelines]]. In case of a mismatch, ask a [https://codereview.qt-project.org/#/admin/groups/1,members Gerrit Administrator] to re-target the affected changes server-side. | ||
# '''Never''' approve changes outside your | # '''Never''' approve changes outside your expertise. | ||
#* While the Approver privilege is granted globally, you are expected to know your limits. | #* 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 are not certain that you know all relevant policies of a particular sub-project, it's a good sign that you should not approve. | ||
#* Do ''not'' approve just because it would be convenient for your colleague across the room/corridor. | #* 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''. | # If you are not really interested in a change, say so and/or give a +1 ''at most''. |
Revision as of 09:16, 13 July 2016
Peer review is a proven method to improve code quality and is therefore strongly encouraged.
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.
- 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.
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 a Gerrit Administrator 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.
- 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.