diff options
| author | Kerri Miller <kerrizor@kerrizor.com> | 2019-07-25 14:02:10 +0000 |
|---|---|---|
| committer | Marcia Ramos <marcia@gitlab.com> | 2019-07-25 14:02:10 +0000 |
| commit | cd2a503db06861595f537bee017fa84ce8f91558 (patch) | |
| tree | 69c9660f87253476ae389514eab68d4782986d00 | |
| parent | 04416fbd2baa8a2cb49c252fac7c14d2f16018a3 (diff) | |
| download | gitlab-ce-cd2a503db06861595f537bee017fa84ce8f91558.tar.gz | |
Add a section of examples
We have a fairly good guide to Code Reviews, but can be improved
by adding a few examples of what a good code review looks like
at GitLab, specifically ones where there is a bit of back and
forth, "nit-picking," etc. This would:
+ help set expectations of newly hired engineers around what our
process looks like when it is functioning what level of scrutiny
their code will be under
+ how we have technical conversations
+ show by example how after you're done crafting a solution, there
can still be extra work done either tidying up code and/or managing
the communication and conversations about your proposed MR
| -rw-r--r-- | doc/development/code_review.md | 25 |
1 files changed, 25 insertions, 0 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 709cd0c806b..b7d74b17eb3 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -365,6 +365,31 @@ Enterprise Edition instance. This has some implications: 1. **Filesystem access** can be slow, so try to avoid [shared files](shared_files.md) when an alternative solution is available. +## Examples + +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):** +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)**: +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)**: +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)**: +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 Largely based on the [thoughtbot code review guide]. |