Code Review Playbook

This page describes various scenarios that can occur when reviewing code, and standardized responses. It is the code review counterpart of the Bug Triaging Playbook.

Description Lacking Information

Often patches only describe what they do, and not what problem they solve, why this is the best solution, how it fits into the bigger picture, what the impact is on users, etc.

Example: D3579

This response should only be used when the patch itself does look useful. If that is not the case, it is likely that there is a bigger issue than just the patch description.

Message Example

Thanks for the patch, it seems quite useful.

To make sure the review is fair and on point, please make sure that the patch description follows the ingredients of a patch. Not only does it make reviewing easier, it also helps when developers later need to figure out what the motivation was for this particular change. Finally it also helps writing the release notes, updating the manual, etc.

Mixing Formatting/Cleanup and Functional Changes

Functional (how Blender works) and non-functional (refactoring, reformatting, commenting) changes should be strictly separated. This response should only be used when the patch itself does look useful. If that is not the case, it is likely that there is a bigger issue than just a mix of functional and non-functional changes.

Message Example (big mixup)

Please separate functional changes (i.e. changes in functionality) from non-functional ones (refactoring, reformatting, commenting). If the feature you're working on requires a cleanup of the existing code, do that cleanup first and submit it as a separate patch. This will make it easier (and thus faster!) to review the changes now, and it will also make it easier to understand the changes later.

Message Example (accidental formatting changes)

Please don't include formatting cleanups in this patch. Please update this patch such that it only contains the functional changes. We're happy with the cleanup, but please submit that as a separate patch.

User feedback in development task

Users often get excited about a new feature / patch and post questions, feedback, examples etc. Design tasks and patches should focus on development though.

Message Example (User comment in development task)

Please keep this task on topic! This task is for developers and should focus on code design and review. Please refrain from further user questions, including asking for a build, asking for a merge ETA, examples, comparison with other software and so on... sIt is also too early for bug reports. Thank you for your understanding.