Code Reviews

Every pull request to Arminius requires a review to ensure quality criteria in the pull request are met and to prevent obvious mistakes from slipping in.

Code reviews can be done by anyone, but to merge a PR a review from at least one code owner is necessary. To help with spreading the load a “review roulette” will randomly assign a reviewer. This however does not mean other team members should not review the PR if they have time. It only exists to force one person to deprioritize other tasks to ensure a timely review. Be aware that the review roulette might assign team members, who are currently on vacation or otherwise can’t take up the review duty. In those cases manual intervention is necessary.

Expectations for a Code Review

We expect code reviews to be:

• timely. A PR author is blocked until a review is done as such we prefer to never have anybody wait more than 2 days for a review.
• empathic. The author has spent a lot of effort usually on the code they are presenting for review. At the same time the reviewer only has so much time to spend on reviews. And both sides might have gaps in understanding of design tradeoffs or parts of the code base. As such the author needs to ensure documentation is clear and helpful as well be open to suggestions and the reviewer needs to be open to their suggestions having been tried already and be aware their feedback might be understood different from what they intended.
• positive. Reviews mostly point out faults. This can bias a review to be something negative. However reviews should be considered something positive, where both participants can learn something and issues can be caught before they cause breakage. As such reviewers and PR authors should be open to learning and praise good solutions or suggestions. The tone should generally be collaborative instead of demanding or degrading.
• helpful. Pointing out an issue is helpful. Providing guidance on how to remediate an issue is a lot more helpful. Only giving feedback about an issue might leave the author with an unclear path on how to “please” the reviewer. If you have concrete suggestions or ideas, state them. It reduces frustration and guess work and speeds up back and forth between parties.

To help with this we expect:

• PRs to be only marked as ready for review, when necessary documentation and changes are finished. Reviewing drafts or WIP can lead to a lot of wasted work. Before undrafting your PR also always do a self review of your own work. There is no need to involve others in a review when you can notice obvious issues yourself already.
• Feedback to be complete and actionable. Unclear feedback can never be resolved and holds up reviews.
• Mutual respect between all review participants. There is no benefit to bringing your ego into a review. The benefit of the team and the code base should always be the highest priority in a review over individual preferences.