Commit 24d5eac0 authored by Ash McKenzie's avatar Ash McKenzie

Merge branch 'docs-development-code-review-efficiency' into 'master'

Add some efficiency steps for code review

See merge request gitlab-org/gitlab!16492
parents d44d72a3 cba40c41
...@@ -185,6 +185,7 @@ without duly verifying them. ...@@ -185,6 +185,7 @@ without duly verifying them.
### Everyone ### Everyone
- Be kind.
- Accept that many programming decisions are opinions. Discuss tradeoffs, which - Accept that many programming decisions are opinions. Discuss tradeoffs, which
you prefer, and reach a resolution quickly. you prefer, and reach a resolution quickly.
- Ask questions; don't make demands. ("What do you think about naming this - Ask questions; don't make demands. ("What do you think about naming this
...@@ -231,6 +232,9 @@ first time. ...@@ -231,6 +232,9 @@ first time.
- Push commits based on earlier rounds of feedback as isolated commits to the - Push commits based on earlier rounds of feedback as isolated commits to the
branch. Do not squash until the branch is ready to merge. Reviewers should be branch. Do not squash until the branch is ready to merge. Reviewers should be
able to read individual updates based on their earlier feedback. able to read individual updates based on their earlier feedback.
- Assign the merge request back to the reviewer once you are ready for another round of
review. If you do not have the ability to assign merge requests, `@`
mention the reviewer instead.
### Assigning a merge request for a review ### Assigning a merge request for a review
...@@ -290,6 +294,10 @@ experience, refactors the existing code). Then: ...@@ -290,6 +294,10 @@ experience, refactors the existing code). Then:
- Seek to understand the author's perspective. - Seek to understand the author's perspective.
- If you don't understand a piece of code, _say so_. There's a good chance - If you don't understand a piece of code, _say so_. There's a good chance
someone else would be confused by it as well. someone else would be confused by it as well.
- Do prefix your comment with "Not blocking:" if you have a small,
non-mandatory improvement you wish to suggest. This lets the author
know that they can optionally resolve this issue in this merge request
or follow-up at a later stage.
- After a round of line notes, it can be helpful to post a summary note such as - After a round of line notes, it can be helpful to post a summary note such as
"LGTM :thumbsup:", or "Just a couple things to address." "LGTM :thumbsup:", or "Just a couple things to address."
- Assign the merge request to the author if changes are required following your - Assign the merge request to the author if changes are required following your
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment