diff options
author | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2016-11-30 12:13:47 +0300 |
---|---|---|
committer | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2016-11-30 13:23:09 +0300 |
commit | 5b052605b7eb7281f88040962e530ac80bbe3774 (patch) | |
tree | 3d19bcc9879d92a5f262b68662362bdc13ebc058 /doc/development/code_review.md | |
parent | 098066050d148deb024fdec6c36bfe9320c674bd (diff) |
Extend code review docs with chapter about the right balance
Diffstat (limited to 'doc/development/code_review.md')
-rw-r--r-- | doc/development/code_review.md | 28 |
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]. |