diff options
| author | Robert Speicher <robert@gitlab.com> | 2016-12-02 08:10:31 +0000 |
|---|---|---|
| committer | Robert Speicher <robert@gitlab.com> | 2016-12-02 08:10:31 +0000 |
| commit | 63b57e5e350c357dbbab98ba7255dd676d50f01f (patch) | |
| tree | ff21944c6c645284279638c7c1568df93b552b9b | |
| parent | 14046b9c734e5e6506d63276f39f3f9d770c3699 (diff) | |
| parent | ebef1a8441b451e2718860538c6e6261ea616bf8 (diff) | |
| download | gitlab-ce-63b57e5e350c357dbbab98ba7255dd676d50f01f.tar.gz | |
Merge branch 'doc/extend-code-review-guidelines' into 'master'
Extend code review docs with chapter about the right balance
[ci skip]
See merge request !7838
| -rw-r--r-- | doc/development/code_review.md | 26 |
1 files changed, 26 insertions, 0 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index c5c23b5c0b8..e1fb8102b67 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -74,6 +74,32 @@ experience, refactors the existing code). Then: - 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 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 makes future changes easier. +- Asking the reviewee to change the design sometimes means the complete rewrite + of the contributed code. It's usually a good idea to ask another 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. A good example is a security fix which should be released as soon as + possible. Asking the 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]. |