Merge branch 'rs-code-review-guide' into 'master'

First pass at a Code Review guide

Largely borrowed from thoughtbot's code review guide, so attribution is
included.

[ci skip]

See merge request !3639

doc/development/README.md

@@ -2,6 +2,8 @@
 
 - [Architecture](architecture.md) of GitLab
 - [CI setup](ci_setup.md) for testing GitLab
+- [Code review guidelines](code_review.md) for reviewing code and having code
+  reviewed.
 - [Gotchas](gotchas.md) to avoid
 - [How to dump production data to staging](db_dump.md)
 - [Instrumentation](instrumentation.md)

doc/development/code_review.md

+# Code Review Guidelines
+
+This guide contains advice and best practices for performing code review, and
+having your code reviewed.
+
+All merge requests for GitLab CE and EE, whether written by a GitLab team member
+or a volunteer contributor, must go through a code review process to ensure the
+code is effective, understandable, and maintainable.
+
+Any developer can, and is encouraged to, perform code review on merge requests
+of colleagues and contributors. However, the final decision to accept a merge
+request is up to one of our merge request "endbosses", denoted on the
+[team page](https://about.gitlab.com/team).
+
+## Everyone
+
+- Accept that many programming decisions are opinions. Discuss tradeoffs, which
+  you prefer, and reach a resolution quickly.
+- Ask questions; don't make demands. ("What do you think about naming this
+  `:user_id`?")
+- Ask for clarification. ("I didn't understand. Can you clarify?")
+- Avoid selective ownership of code. ("mine", "not mine", "yours")
+- Avoid using terms that could be seen as referring to personal traits. ("dumb",
+  "stupid"). Assume everyone is attractive, intelligent, and well-meaning.
+- Be explicit. Remember people don't always understand your intentions online.
+- Be humble. ("I'm not sure - let's look it up.")
+- Don't use hyperbole. ("always", "never", "endlessly", "nothing")
+- Be careful about the use of sarcasm. Everything we do is public; what seems
+  like good-natured ribbing to you and a long-time colleague might come off as
+  mean and unwelcoming to a person new to the project.
+- Consider one-on-one chats or video calls if there are too many "I didn't
+  understand" or "Alternative solution:" comments. Post a follow-up comment
+  summarizing one-on-one discussion.
+
+## Having your code reviewed
+
+- The first reviewer of your code is _you_. Before you perform that first push
+  of your shiny new branch, read through the entire diff. Does it make sense?
+  Did you include something unrelated to the overall purpose of the changes? Did
+  you forget to remove any debugging code?
+- Be grateful for the reviewer's suggestions. ("Good call. I'll make that
+  change.")
+- Don't take it personally. The review is of the code, not of you.
+- Explain why the code exists. ("It's like that because of these reasons. Would
+  it be more clear if I rename this class/file/method/variable?")
+- Extract unrelated changes and refactorings into future merge requests/issues.
+- Seek to understand the reviewer's perspective.
+- Try to respond to every comment.
+- Push commits based on earlier rounds of feedback as isolated commits to the
+  branch. Do not squash until the branch is ready to merge. Reviewers should be
+  able to read individual updates based on their earlier feedback.
+
+## Reviewing code
+
+Understand why the change is necessary (fixes a bug, improves the user
+experience, refactors the existing code). Then:
+
+- Communicate which ideas you feel strongly about and those you don't.
+- Identify ways to simplify the code while still solving the problem.
+- Offer alternative implementations, but assume the author already considered
+  them. ("What do you think about using a custom validator here?")
+- Seek to understand the author's perspective.
+- If you don't understand a piece of code, _say so_. There's a good chance
+  someone else would be confused by it as well.
+- 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 ("Merge when build
+  succeeds" is fine).
+
+## Credits
+
+Largely based on the [thoughtbot code review guide].
+
+[thoughtbot code review guide]: https://github.com/thoughtbot/guides/tree/master/code-review
+
+---
+
+[Return to Development documentation](README.md)