First pass at a Code Review guide
Largely borrowed from thoughtbot's code review guide, so attribution is included. [ci skip]
This commit is contained in:
parent
dfe4c69ea8
commit
c7ec5929b1
|
@ -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)
|
||||
|
|
|
@ -0,0 +1,75 @@
|
|||
# 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")
|
||||
- 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)
|
Loading…
Reference in New Issue