Code Review Practices

As mentioned in our Pull Request Practices, everyone is encouraged to participate in code reviews regardless of experience level. Code reviews are not only helpful for the person's code being reviewed. It's a great learning experience for the reviewer themselves!

Check out our Learning Resources to read up on collaboration best practices and add your recommend resources.

Our Code Review Commandments

Our use of the term commandments is inspired by The 10 Commandments of Navigating Code Reviews by Angie Jones.

Collaboration

• The Contributor Convenant applies in every communication channel we have in our contributor community, code reviews are no exception.
• Code reviews are a learning experience for all parties involved so be cognizant of that. It's not an opportunity to shame others for knowing something or implementing something a certain way.
• A lot of back and forth conversations in GitHub can create tension, so we recommend taking the conversation to another place if it's difficult to get your point across or understand something. Examples: one-on-one conversations on Slack or a voice/video call on Google Hangouts.
• Ask lots of questions as both reviewers and reviewees! Making unconfirmed or false assumptions often leads to the negative repercussions of unconscious bias and conscious bias.
• Be supportive! Kind gestures like words of encouragement goes a long way.
• Please remember that you are not the code you write. No one has the answers to everything and writes code perfectly.

Code Quality

• Code must adhere to our Frontend Practices and Backend Practices
• Test coverage is required and must follow our Automated Testing practices.
• We set up CircleCI, a continuous integration and delivery tool, to automatically run our tests and Codeclimate, a tool to run automated code quality checks, in pull requests. To save time, a reviewer does not have to point out syntax errors as the reviewee should update their code to meet Codeclimate's expectations. If a reviewee forgets to update their code based on Codeclimate's feedback before merging, a reviewer should kindly remind them to.
• Sometimes Codeclimate is too picky! A PR is only mergeable to master when the CircleCI tests go green and a reviewer has approved it. If you're unable to make the Codeclimate tests go green because what it's asking for is unrelated to your changes or not a good ask, make a note of it in a PR comment. We will update our Codeclimate settings accordingly.
• When describing a way to solve a coding problem, be concise and clear. You may link to articles that could help emphasize your point but don't overdo it. No one wants to be just told to read the manual.

Review Cycles

• Please use GitHub's nifty tools on requesting reviewers. It's there for a reason!
• Reviewers should be explicit about whether they approve a review or require changes to be made.
• Do not amend commits if your change is unrelated to that commit, create a new commit.
• After the reviewee has made changes after the first round of reviews, the reviewee can re-add someone as the reviewer and that will trigger a notification for them to review again.
• The Pull Requests page on GitHub is a nifty tool of keeping track of what issues are assigned to you and what PRs you were requested to review.
• If a reviewer hasn't reviewed your code after a certain period of time you need it to be reviewed, kindly @ them in a comment in the PR with a reminder.
• Oh no! You haven't heard back from your reviewer in a while. Unfortunately and fortunately, that's the nature of open source software. We're all working on this project voluntarily and it's important to create boundaries. As a mental health project, we gotta practice what we preach! Don't harass the reviewer to review your code. Post in #dev on Slack that you need a new pair of eyes to review.

Merging to Master

• Once the PR is approved, the reviewer can merge it or whoever else.
• Monitor for a successful merge to master by checking the CircleCI build. You can find it for each commit in master here
• If the build goes green, you're done! Your changes are deployed to master!
• If it goes red, don't panic! There two scenarios:
  • Flakey failing tests: we've noticed that acceptance tests involving translations will sometimes fail because of a race condition. If that's the case, rebuild the test in CircleCI.
  • Valid failing tests: the first thing to do is to revert the commit on master. Open a new PR when you've fixed the failing tests. Experiencing valid failing tests after merging to master should be unlikely because your PR must have passed tests before being merged to master.

What happens after? Well, production deployment!