diff options
Diffstat (limited to 'doc/development/code_review.md')
| -rw-r--r-- | doc/development/code_review.md | 19 |
1 files changed, 12 insertions, 7 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index fe395dc2304..d856541bd91 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -24,7 +24,7 @@ uncovered edge cases. The default approach is to choose a reviewer from your group or team for the first review. This is only a recommendation and the reviewer may be from a different team. However, it is recommended to pick someone who is a [domain expert](#domain-experts). -If your merge request touches more than one domain (for example, Dynamic Analysis and GraphQL), ask for reviews from an expert from each domain. +If your merge request touches more than one domain (for example, Dynamic Analysis and GraphQL), ask for reviews from an expert from each domain. You can read more about the importance of involving reviewer(s) in the section on the responsibility of the author below. @@ -107,11 +107,11 @@ with [domain expertise](#domain-experts). be **approved by a [frontend foundations member](https://about.gitlab.com/direction/create/ecosystem/frontend-ux-foundations/)**. - If the license used by the new library hasn't been approved for use in GitLab, the license must be **approved by a [legal department member](https://about.gitlab.com/handbook/legal/)**. - More information about license compatiblity can be found in our + More information about license compatibility can be found in our [GitLab Licensing and Compatibility documentation](licensing.md). 1. If your merge request includes adding a new UI/UX paradigm (*1*), it must be **approved by a [UX lead](https://about.gitlab.com/company/team/)**. -1. If your merge request includes a new dependency or a filesystem change, it must be +1. If your merge request includes a new dependency or a file system change, it must be **approved by a [Distribution team member](https://about.gitlab.com/company/team/)**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/development/enablement/distribution/#how-to-work-with-distribution) for more details. 1. If your merge request includes documentation changes, it must be **approved by a [Technical writer](https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments)**, based on @@ -121,6 +121,8 @@ with [domain expertise](#domain-experts). 1. If your merge request only includes end-to-end changes (*3*) **or** if the MR author is a [Software Engineer in Test](https://about.gitlab.com/handbook/engineering/quality/#individual-contributors), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa)** 1. If your merge request includes a new or updated [application limit](https://about.gitlab.com/handbook/product/product-processes/#introducing-application-limits), it must be **approved by a [product manager](https://about.gitlab.com/company/team/)**. 1. If your merge request includes Product Intelligence (telemetry or analytics) changes, it should be reviewed and approved by a [Product Intelligence engineer](https://gitlab.com/gitlab-org/growth/product_intelligence/engineers). +1. If your merge request includes an addition of, or changes to a [Feature spec](testing_guide/testing_levels.md#frontend-feature-tests), it must be **approved by a [Quality maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_maintainers_qa) or [Quality reviewer](https://about.gitlab.com/handbook/engineering/projects/#gitlab_reviewers_qa)**. +1. If your merge request introduces a new service to GitLab (Puma, Sidekiq, Gitaly are examples), it must be **approved by a [product manager](https://about.gitlab.com/company/team/)**. See the [process for adding a service component to GitLab](adding_service_component.md) for details. - (*1*): Please note that specs other than JavaScript specs are considered backend code. - (*2*): We encourage you to seek guidance from a database maintainer if your merge @@ -177,8 +179,11 @@ warrant a comment could be: 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. +- Adding TODO comments (referenced above) directly to the source code unless the reviewer requires + you to do so. If TODO comments are added due to an actionable task, + [include a link to the relevant issue](code_comments.md). +- Adding comments which only explain what the code is doing. If non-TODO comments are added, they should + [_explain why, not what_](https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/). - 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. @@ -340,7 +345,7 @@ experience, refactors the existing code). Then: convey your intent. - For non-mandatory suggestions, decorate with (non-blocking) so the author knows they can optionally resolve within the merge request or follow-up at a later stage. - - There's a [Chrome/Firefox addon](https://gitlab.com/conventionalcomments/conventional-comments-button) which you can use to apply [Conventional Comment](https://conventionalcomments.org/) prefixes. + - There's a [Chrome/Firefox add-on](https://gitlab.com/conventionalcomments/conventional-comments-button) which you can use to apply [Conventional Comment](https://conventionalcomments.org/) prefixes. - After a round of line notes, it can be helpful to post a summary note such as "Looks good to me", or "Just a couple things to address." - Assign the merge request to the author if changes are required following your @@ -565,7 +570,7 @@ A good example of collaboration on an MR touching multiple parts of the codebase ### Credits -Largely based on the [thoughtbot code review guide](https://github.com/thoughtbot/guides/tree/master/code-review). +Largely based on the [`thoughtbot` code review guide](https://github.com/thoughtbot/guides/tree/master/code-review). --- |