summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGrzegorz Bizon <grzesiek.bizon@gmail.com>2016-11-30 10:13:47 +0100
committerGrzegorz Bizon <grzesiek.bizon@gmail.com>2016-11-30 11:23:09 +0100
commit5b052605b7eb7281f88040962e530ac80bbe3774 (patch)
tree3d19bcc9879d92a5f262b68662362bdc13ebc058
parent098066050d148deb024fdec6c36bfe9320c674bd (diff)
downloadgitlab-ce-5b052605b7eb7281f88040962e530ac80bbe3774.tar.gz
Extend code review docs with chapter about the right balance
-rw-r--r--doc/development/code_review.md28
1 files changed, 27 insertions, 1 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index c5c23b5c0b8..709cefcb5c6 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -70,10 +70,36 @@ experience, refactors the existing code). Then:
- 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."
- Avoid accepting a merge request before the build succeeds. Of course, "Merge
- When Build Succeeds" (MWBS) is fine.
+ When Pipeline Succeeds" is fine.
- If you set the MR to "Merge When Build Succeeds", you should take over
subsequent revisions for anything that would be spotted after that.
+## The right balance
+
+One of the most difficult things during the code review is finding the right
+balance in how deep the reviewer can interfere with the code created by a
+reviewee.
+
+- Learning how to find the right balance takes time, that is why we have
+ minibosses that become merge request endbosses after some time spent on
+ reviewing merge requests.
+- Finding bugs and improving code style is important, but thinking about good
+ design is important as well. Building abstractions and good design is what
+ makes it possible to hide complexity and is a leverage for the future work.
+- Asking reviewee to change the design sometimes means the complete rewrite of
+ the contributed code. It is usually a good idea to ask other merge request
+ endboss before doing it, but have the courage to do it when you believe it is
+ important.
+- There is a difference in doing things right and doing things right now.
+ Ideally, we should do the former, but in the real world we need the latter as
+ well. The good example is a security fix which should be released as soon as
+ possible. Asking reviewee to do the major refactoring in the merge request
+ that is an urgent fix should be avoided.
+- Doing things well today is usually better than doing something perfectly
+ tomorrow. Shipping a kludge today is usually worse than doing something well
+ tomorrow. When you are not able to find the right balance, ask other people
+ about their opinion.
+
## Credits
Largely based on the [thoughtbot code review guide].