Review Policy: Difference between revisions

From Qt Wiki
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 expertize.
# '''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

  1. 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).
  2. 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.
  3. Discuss objections. Do not override a -1 unless there is a broad expert consensus that the objection is unfounded.
  4. Do not ignore/fight the Early Warning System. Justify each override.
  5. 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
      1. at least one review (+1) from somebody else is present, and
      2. nobody else who could approve (+2) the change can be produced within reasonable time.

As the Reviewer

  1. Ensure compliance with the Commit Policy.
  2. 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.
  3. 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.
  4. If you are not really interested in a change, say so and/or give a +1 at most.