Account confirmers, Anti-spam team, Confirmed users, Bureaucrats and Sysops emeriti
4,925
edits
Code Review: Difference between revisions
Jump to navigation
Jump to search
Highlight key points and improve readability.
(→Patch-author Guidelines: add a few more) |
(Highlight key points and improve readability.) |
||
| Line 1: | Line 1: | ||
== Reviewer Guidelines == | == Reviewer Guidelines == | ||
# Prioritize. Reviewing code is more valuable than writing code as it results in higher overall project activity. If you find you can't write code | # '''Prioritize reviews highly.''' Reviewing code is more valuable than writing code as it results in higher overall project activity. If you find you can't write code any more due to prioritizing reviews over coding, appoint more reviewers. | ||
# Communicate. If you are an active contributor, you should | # '''Communicate.''' If you are an active contributor, you should respond to a review request within one working day of getting it, either with a review, a deadline by which you promise to do the review, or a polite refusal. If you think a patch is lower priority than your other work communicate that. | ||
# Silence is not an option. If you think saying nothing is better than admitting than you | # '''Silence is not an option.''' If you think saying nothing is better than admitting than you won't get to the patch for a while, that's being [https://en.wikipedia.org/wiki/Passive-aggressive_behavior passive-aggressive]. This is not a good way to build a happy coding community. Holding back bad code is a feature, not a bug - so do it, quickly and politely. | ||
== Patch | == Patch Author Guidelines == | ||
There are some things that make it easier to get a good, prompt review, and to improve the quality of the patch you submit. Help your reviewers review your patches quickly by making patches easy to review: | There are some things that make it easier to get a good, prompt review, and to improve the quality of the patch you submit. Help your reviewers review your patches quickly by making patches easy to review: | ||
* Split mass-changes or mechanical changes into a separate patch from the substantive changes. | * '''Split out mass-changes or mechanical changes''' into a separate patch from the substantive changes. | ||
* | * '''Separate patches into conceptually-separate pieces''' for review purposes (even if you then later collapse them into a single changeset to push), if technically possible. Any time you're requesting review from multiple people on a single huge diff, chance are splitting it might have been a good idea. | ||
* | * '''Make the summary of the bug reflect the problem''' rather than the solution, so there's a clear description of what the patch is trying to fix | ||
* | * '''Write good commit messages''' that describe what's changing between old and new code (which, if it can't be summarized in less than about 100-150 characters, should have a short summary on the first line and a longer description on later lines) | ||
* | * '''Write good code comments''' that describe the state of the new code, and if the patch is of nontrivial size, point to the important comments in the non-first lines of the commit message | ||
* | * '''Address all comments from previous reviews''' (either by fixing as requested, or explaining why you haven't) before requesting another review. It's very common to just ignore (miss?) some of the review comments, which means the reviewer then needs to triple-check that all the things they pointed out got fixed. | ||
* | * '''Provide an interdiff''' for second and subsequent patches when requesting review for them. This is so that the reviewer can verify that the changes you made match what they asked for. (Bugzilla's interdiff is totally unsuitable for this purpose, unfortunately, because it fails so often because it doesn't have access to the original code.) | ||
* | * '''If your request only relates to part of the patch, say so clearly.''' | ||
* | * '''Don't request review from people who are labelled as being away''' in Bugzilla. | ||
== Bugzilla Improvements == | == Potential Bugzilla Improvements == | ||
* In addition to r+/r- add | * In addition to r+/r- add a '''Review ETA''' field where the requested reviewer can specify when the patch will get reviewed. Bugzilla should send reminder emails based on that flag. | ||
* Provide information about reviewers when asking for r? | * Provide information about reviewers when asking for r? | ||
** Add a notion of being away to | ** Add a notion of being away to Bugzilla accounts, or integrate with a Time Off tracking app. | ||
** Display number of outstanding reviews | ** Display a user's number of outstanding reviews | ||
* Add a 'low-priority' patch flag | * Add a 'low-priority' patch flag | ||
* Some integration with a formatting checker | * Some integration with a formatting checker | ||