Review Policy: Difference between revisions
Jump to navigation
Jump to search
(long live the review policy!) |
m (give sections numbers, so it's easier to refer to them) |
||
| (6 intermediate revisions by 2 users not shown) | |||
| Line 1: | Line 1: | ||
[[Category:Developing_Qt::Guidelines]] | [[Category:Developing_Qt::Guidelines]] | ||
[[Category:Developing_Qt::Process]] | |||
[[Category:Tools::Gerrit]] | |||
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. | ||
== As the Contributor == | == 1. As the Contributor == | ||
# Invite relevant reviewers. | # Invite relevant reviewers. | ||
| Line 11: | Line 13: | ||
#* 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. | #* 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. | #* 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. | # 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 ignore/fight the [[Early Warning System]]. Justify each override. | ||
| Line 19: | Line 22: | ||
#*# nobody else who could approve (+2) the change can be produced within reasonable time. | #*# nobody else who could approve (+2) the change can be produced within reasonable time. | ||
== As the Reviewer == | == 2. As the Reviewer == | ||
# 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 | # Make sure the target branch complies with the [[Branch Guidelines]]. In case of a mismatch, ask the user to [[Gerrit_Introduction#Moving_Changes_to_Different_Branches|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. | ||
#* 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. | #* 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''. | ||
Latest revision as of 17:54, 27 October 2020
Peer review is a proven method to improve code quality and is therefore strongly encouraged.
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.