ITK/Release 4/Modularization/Code Reviews/Process

This page summarizes a process proposal for managing the code reviews of the modules in ITK.

Overview

A Git repository will be used to host the comments from reviewers.

• Kitware will create a separate Git repository containing
  • One text file for each one of the files in the current ITK Manifest.
  • The files will be named after the ITK files. For example
    • itkImage.h.txt will be the code review file for itkImage.h
• The repository will contain at the top level, one directory per contractor, and under that directory we will replicate the monolithic directory tree structure of ITK with only the files that have been assigned to that contractor.
• This text files will be populated by the code reviewers with comments describing their observations during the code review process.

Workflow

This section describes the life-cycle of a file when going through the code review process.

Two concepts are introduced here:

• Status: the current state of a given file in the code review process
• Rating: the severity of the modifications needed by this file

Graphical Overview

Review Status

For each file, a contractor will review the code following the Checklist and specify one of the following potential status

• Pending: No review has been performed yet.
• Rated: Review has been performed and it resulted into actions to be performed by a developer.
• Done: Corrective actions have been taken. No further action required.

Transition Rules

• All files start in the Pending state.
• From Pending to Rated: A reviewer will review the file following the Checklist, and identify actions to be taken, and will assign a developer to look into the details. The reviewer will assign a rating according to the list below.
• From Rated to Done: The assigned developer will communicate back to the reviewer when the actions have been taken, the reviewer will verify the changes and, if satisfied, will set the status to Done.
• From Pending to Done: A reviewer may make this transition if the rating is Good as defined in the rating section below.

Rating

• Good: The file has been reviewed, and no change was required.
• Minor: Minor typos. In this case the reviewer will fix the problems directly (and still submit them to Gerrit). Minor changes are defined a those that do not influence the API. (including doxygen documentation).
• Major: File needs refactoring or major bug fixing. The reviewer must log a mantis and include the Bug number in the comments in the .txt review file, and the developer to which the bug has been assigned.
• Remove: File must be removed (broke, obsolete, irreparable).

Refactoring

For files to be refactored, that is, files that were rated as:

• Major
• Remove

the following process should be followed.

• The reviewer will log a JIRA Bug (in the standard ITK Bug tracker)
• The reviewer will assign this bug to the developer who may be more knowledgeable about the file, e.g. the first committer of the file).
• From there, the bug will be managed in the standard way.
  • Dependencies with other changes will be discussed at this point
  • The assigned developer will discuss fixing alternatives
  • Will implement one of such alternatives
  • The developer will push a patch to Gerrit
  • Any API issues will be managed following the Migration guide documentation procedure
  • The patch will go through code review
  • Eventually will be merged into master
• Once the fix is merged, the bug will be closed, and the developer will notify the reviewer
• The reviewer will mark the issue as "Done" (using the adopted implementation: Git, MANTIS or Trac).

Implementation Procedure

Git-Based

  https://github.com/InsightSoftwareConsortium/itk-retroactive-review

We will use a parallel Git repository for hosting the content of the reviews in free format text files.

• The repository has a mirror copy of the Modular ITK directory tree structure.
• For each ITK source code file, this Git repository contains a text file.
  • The filename matches one-to-one the filenames in ITK, plus the extension .txt.
    • For example, there will be a file itkImage.h.txt for the reviews of the file itkImage.h.
• The presence of the empty .txt files will indicate a Pending status
• Reviewers will edit those files, as they review the ITK code, and will populate the files with the ratings and any additional comments that they consider relevant. The changes will be committed, and merged to the master branch of this dedicated repository.