canmove, Confirmed users, Bureaucrats and Sysops emeriti
1,334
edits
Code Review: Difference between revisions
Jump to navigation
Jump to search
→Patch-author Guidelines: add a few more
(→Patch-author Guidelines: add a few more) |
|||
| Line 11: | Line 11: | ||
* Split mass-changes or mechanical changes into a separate patch from the substantive changes. | * Split mass-changes or mechanical changes into a separate patch from the substantive changes. | ||
* If possible, separate patches into conceptually-separate pieces for review purposes (even if you then later collapse them into a single changeset to push). Any time you're requesting review from multiple people on a single huge diff, chance are splitting it might have been a good idea. | * If possible, separate patches into conceptually-separate pieces for review purposes (even if you then later collapse them into a single changeset to push). 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 so that 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 | |||
* Actually address the review comments 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. | * Actually address the review comments 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. | ||
* When requesting a second review on a patch, provide an interdiff so that the reviewer can just 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. | * When requesting a second review on a patch, provide an interdiff so that the reviewer can just 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. | ||