diff options
Diffstat (limited to 'doc/development/code_review.md')
| -rw-r--r-- | doc/development/code_review.md | 20 |
1 files changed, 14 insertions, 6 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index ce9f794c1a3..f616eb90bda 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -185,6 +185,7 @@ without duly verifying them. ### Everyone +- Be kind. - Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly. - Ask questions; don't make demands. ("What do you think about naming this @@ -231,6 +232,9 @@ first time. - 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 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 @@ -254,7 +258,7 @@ even when this may negatively impact their other tasks and priorities. Doing so allows everyone involved in the merge request to iterate faster as the context is fresh in memory, improves contributors' experiences significantly, and gives authors more time to address feedback and iterate on their work before -the [feature freeze](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md#feature-freeze-on-the-7th-for-the-release-on-the-22nd). +the [feature freeze](https://gitlab.com/gitlab-org/gitlab/blob/master/PROCESS.md#feature-freeze-on-the-7th-for-the-release-on-the-22nd). A turnaround time of two working days is usually acceptable, since engineers will typically have other things to work on while they're waiting for review, @@ -262,7 +266,7 @@ but don't hesitate to ask the author if it's unclear what time frame would be acceptable, how urgent the review is, or how significant the blockage. Authors are also encouraged to provide this information up-front to reviewers, but are expected to be mindful of the [guidelines on when to ask for review on MRs that -are intended to go in before the feature freeze](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md#between-the-1st-and-the-7th), +are intended to go in before the feature freeze](https://gitlab.com/gitlab-org/gitlab/blob/master/PROCESS.md#between-the-1st-and-the-7th), and realistic in their expectations if these were not followed. If you don't think you'll be able to review a merge request within a reasonable @@ -290,6 +294,10 @@ experience, refactors the existing code). Then: - Seek to understand the author's perspective. - 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. +- 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 "LGTM :thumbsup:", or "Just a couple things to address." - Assign the merge request to the author if changes are required following your @@ -393,25 +401,25 @@ Enterprise Edition instance. This has some implications: How code reviews are conducted can surprise new contributors. Here are some examples of code reviews that should help to orient you as to what to expect. -**["Modify `DiffNote` to reuse it for Designs"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/13703):** +**["Modify `DiffNote` to reuse it for Designs"](https://gitlab.com/gitlab-org/gitlab/merge_requests/13703):** It contained everything from nitpicks around newlines to reasoning about what versions for designs are, how we should compare them if there was no previous version of a certain file (parent vs. blank `sha` vs empty tree). -**["Support multi-line suggestions"](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25211)**: +**["Support multi-line suggestions"](https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/25211)**: The MR itself consists of a collaboration between FE and BE, and documenting comments from the author for the reviewer. There's some nitpicks, some questions for information, and towards the end, a security vulnerability. -**["Allow multiple repositories per project"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10251)**: +**["Allow multiple repositories per project"](https://gitlab.com/gitlab-org/gitlab/merge_requests/10251)**: ZJ referred to the other projects (workhorse) this might impact, suggested some improvements for consistency. And James' comments helped us with overall code quality (using delegation, `&.` those types of things), and making the code more robust. -**["Support multiple assignees for merge requests"](https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/10161)**: +**["Support multiple assignees for merge requests"](https://gitlab.com/gitlab-org/gitlab/merge_requests/10161)**: A good example of collaboration on an MR touching multiple parts of the codebase. Nick pointed out interesting edge cases, James Lopes also joined in raising concerns on import/export feature. ### Credits |