diff options
| author | Ash McKenzie <amckenzie@gitlab.com> | 2019-09-05 02:28:19 +0000 |
|---|---|---|
| committer | Ash McKenzie <amckenzie@gitlab.com> | 2019-09-05 02:28:19 +0000 |
| commit | 0fca70a40f5067b333ff62e556754b9dd04e26f9 (patch) | |
| tree | d56aa958a6105089757fa5767fc4f85be7b98da5 | |
| parent | 0fad0ed864c5b0f65956d1794cee2f9bd7bfa608 (diff) | |
| parent | 53275ad1a2d168f946b563dcee97610a358ec26d (diff) | |
| download | gitlab-ce-0fca70a40f5067b333ff62e556754b9dd04e26f9.tar.gz | |
Merge branch 'mj/code-review-avoid' into 'master'
What to avoid in code review
See merge request gitlab-org/gitlab-ce!32592
| -rw-r--r-- | doc/development/code_review.md | 9 |
1 files changed, 7 insertions, 2 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 80890d96159..bcfc0734c06 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -116,8 +116,13 @@ warrant a comment could be: - Any benchmarking performed to complement the change - Potentially insecure code -Do not add these comments directly to the source code, unless the -reviewer requires you to do so. +Avoid: + +- Adding comments (referenced above, or TODO items) directly to the source code unless the reviewer requires you to do so. If the comments are added due to an actionable task, +a link to an issue must be included. +- Assigning merge requests with failed tests to maintainers. If the tests are failing and you have to assign, ensure you leave a comment with an explanation. +- Excessively mentioning maintainers through email or Slack (if the maintainer is reachable +through Slack). If you can't assign a merge request, `@` mentioning a maintainer in a comment is acceptable and in all other cases assigning the merge request is sufficient. This [saves reviewers time and helps authors catch mistakes earlier](https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/index.html#__RefHeading__97_174136755). |