User:Dfelinto/Drafts/PatchReview

Patch Review

Any patch starts with a well defined design (product design and engineer plan). This is what a module owner will look at and decide whether the contributor can move on with the implementation.

☑ Product Design
☑ Engineer Plan

It is ok for a developer to do initial code for engineer or usability exploration. They can still get feedback for these, but never on the code level itself, only on the design (project/engineer) level.

Once the designs are approved, the module owner can assign a developer (often a module member) to review the code itself.

☑ Documentation
☑ Semi-Automated tests
☑ Regression tests
☑ Quality assurance
☑ Code quality

Once the checklist is done, patch can be sent to the module owner again, for final approval, but the module owner shouldn’t be required to code review it. In exceptional cases the chief architect can be involved at this stage.

The patch status can be clearly communicated visually. Here yellow means current on review, green approved, red means we haven't/won't review it until the yellow stage is reviewed.

Patch Review Steps

☑ - Required / ☐ - Case-by-case

General

☑ Cover letter - What is the intention of this project, tips on where/how to review the code.
☐ Test file - Simple and complex files for testing and UI/UX workflow evaluation.

Design

☑ Vision - Why this project, what problem does it solve.
☑ Use cases
☐ UX / UI Mockup

Engineer Plan

☑ Areas of code and their relationship
☑ General architecture
☐ New libraries

Documentation

☐ User manual
☑ Commit message - Clearly separated why/what on user level as well as development.
☐ Release note - Snippet of what will then be the release note.
☐ Python examples - API doc examples, or Python templates.

Semi-Automated Tests

☑ Continuous Integration - Code style, unit tests, clang, build.
☑ Descriptive names - Variables, classes, structures, functions.
☑ Commented code - Why not what mostly, "what" only in a high level.
☑ Doversion - If not, at least acknowledged

Regression Tests

☑ Coverage - Follow Blender unit test coverage scope.
☑ Speed - Test should run fast.
☐ Size - Files should be small.

Quality Assurance

☑ UI / UX - User interface and usability.
☑ Functionality - Address what is supposed to do.
☑ Undo - Performance and reliability on undo.
☑ Stress Test - Try to get it to crash.

Code Quality

☑ API consistency.
☑ Using existing code - Matrix multiplication code, ...
☑ Well defined function scopes
☑ Assert usage
☑ Recovery from errors - NULL checks, warnings to users.
☑ Architecture compliance - Notifier system, racing condition, cache, contention, module domain (kernel, blenlib, editor, intern, extern separation).

Internal Patch Review

Even among hired developers the patch review is an essential part of our collaboration workflow. A few high topics to be elaborated later:

• Review immediately unless working in time sensitive tasks.
• Pair review (1-2h/week) of big patches (internal or external), up to the competence of the involved team.