Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2018-10-17 20:38:45 +0300
committerDouwe Maan <douwe@gitlab.com>2018-10-17 20:38:45 +0300
commit2a631de547b230e52daf591cbbf31e0b1e953d45 (patch)
treeb3adc6228a3e4fa3cd259534e68b6b94797a77ac /doc/development/code_review.md
parenta706b3735ef01072401dd1ee67ba0586a2fdf61c (diff)
Strongly recommend involving a domain expert, especially when in doubt.
Diffstat (limited to 'doc/development/code_review.md')
-rw-r--r--doc/development/code_review.md12
1 files changed, 8 insertions, 4 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 4d3a817e78b..3fe79943fdc 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -13,8 +13,8 @@ You are strongly encouraged to get your code **reviewed** by a
[reviewer](https://about.gitlab.com/handbook/engineering/#reviewer) as soon as
there is any code to review, to get a second opinion on the chosen solution and
implementation, and an extra pair of eyes looking for bugs, logic problems, or
-uncovered edge cases. The reviewer can be from a different team, but it is often
-useful to pick someone who knows the domain well. You can read more about the
+uncovered edge cases. The reviewer can be from a different team, but it is
+recommended to pick someone who knows the domain well. You can read more about the
importance of involving reviewer(s) in the section on the responsibility of the author below.
If you need some guidance (e.g. it's your first merge request), feel free to ask
@@ -48,7 +48,7 @@ from other teams than your own.
The responsibility to find the best solution and implement it lies with the
merge request author.
-Before assigning a merge request to maintainer for approval and merge, they
+Before assigning a merge request to a maintainer for approval and merge, they
should be confident that it actually solves the problem it was meant to solve,
that it does so in the most appropriate way, that it satisfies all requirements,
and that there are no remaining bugs, logical problems, or uncovered edge cases.
@@ -57,7 +57,7 @@ a passing CI pipeline to avoid unnecessary back and forth.
To reach the required level of confidence in their solution, an author is expected
to involve other people in the investigation and implementation processes as
-appropriate:
+appropriate.
They are encouraged to reach out to domain experts to discuss different solutions
or get an implementation reviewed, to product managers and UX designers to clear
@@ -65,6 +65,10 @@ up confusion or verify that the end result matches what they had in mind, to
database specialists to get input on the data model or specific queries, or to
any other developer to get an in-depth review of the solution.
+If an author is unsure if a merge request needs a domain expert's opinion, that's
+usually a pretty good sign that it does, since without it the required level of
+confidence in their solution will not have been reached.
+
### The responsibility of the maintainer
Maintainers are responsible for the overall health, quality, and consistency of