Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
0
Merge Requests
0
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
Léo-Paul Géneau
gitlab-ce
Commits
2a631de5
Commit
2a631de5
authored
Oct 17, 2018
by
Douwe Maan
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Strongly recommend involving a domain expert, especially when in doubt.
parent
a706b373
Changes
1
Hide whitespace changes
Inline
Side-by-side
Showing
1 changed file
with
8 additions
and
4 deletions
+8
-4
doc/development/code_review.md
doc/development/code_review.md
+8
-4
No files found.
doc/development/code_review.md
View file @
2a631de5
...
@@ -13,8 +13,8 @@ You are strongly encouraged to get your code **reviewed** by a
...
@@ -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
[
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
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
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
uncovered edge cases. The reviewer can be from a different team, but it is
useful to pick someone who knows the domain well. You can read more about the
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.
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
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.
...
@@ -48,7 +48,7 @@ from other teams than your own.
The responsibility to find the best solution and implement it lies with the
The responsibility to find the best solution and implement it lies with the
merge request author.
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,
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,
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.
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.
...
@@ -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 reach the required level of confidence in their solution, an author is expected
to involve other people in the investigation and implementation processes as
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
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
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
...
@@ -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
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.
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
### The responsibility of the maintainer
Maintainers are responsible for the overall health, quality, and consistency of
Maintainers are responsible for the overall health, quality, and consistency of
...
...
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment