ITK/Release 4/Modularization/Code Reviews/Process: Difference between revisions
From KitwarePublic
< ITK | Release 4 | Modularization | Code Reviews
Jump to navigationJump to search
No edit summary |
Daviddoria (talk | contribs) m (moved ITK Release 4/Modularization/Code Reviews/Process to ITK/Release 4/Modularization/Code Reviews/Process) |
||
| (20 intermediate revisions by 3 users not shown) | |||
| Line 1: | Line 1: | ||
This page summarizes a process proposal for managing the code reviews of the modules in ITK. | |||
__TOC__ | |||
= Overview = | = Overview = | ||
A Git repository will be used to host the comments from reviewers. | |||
* Kitware will create a separate Git repository containing | * Kitware will create a separate Git repository containing | ||
| Line 16: | Line 12: | ||
*** itkImage.h.txt will be the code review file for itkImage.h | *** 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. | * 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 the | * 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 == | == Graphical Overview == | ||
| Line 31: | Line 30: | ||
Rated; | Rated; | ||
Done; | Done; | ||
Pending -> Rated; | Pending -> Rated [label="Major"]; | ||
Pending -> Done; | Pending -> Rated [label="Remove"]; | ||
Rated -> Done; | Pending -> Done [label="Minor,Fixed"]; | ||
Rated -> Done [label="Major,Fixed"]; | |||
Pending -> Done [label="Good"]; | |||
} | } | ||
</graphviz> | </graphviz> | ||
| Line 39: | Line 40: | ||
== Review Status == | == Review Status == | ||
For each file, a contractor will review the code following the | For each file, a contractor will review the code following the [[ITK_Release_4/Modularization/Code Reviews/Checklist|Checklist]] and specify one of the following potential status | ||
* '''Pending''': No review has been performed yet. | * '''Pending''': No review has been performed yet. | ||
| Line 48: | Line 49: | ||
* All files start in the '''Pending''' state. | * All files start in the '''Pending''' state. | ||
* From '''Pending''' to '''Rated''': A reviewer will review the file following the | * From '''Pending''' to '''Rated''': A reviewer will review the file following the [[ITK_Release_4/Modularization/Code Reviews/Checklist|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 '''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. | * From '''Pending''' to '''Done''': A reviewer may make this transition if the rating is '''Good''' as defined in the rating section below. | ||
| Line 56: | Line 57: | ||
* '''Good''': The file has been reviewed, and no change was required. | * '''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). | * '''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 | * '''Major''': File needs refactoring or major bug fixing. The reviewer must log a JIRA issue and include the issue 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). | * '''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 following | * 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, JIRA 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. | |||
Latest revision as of 16:00, 9 December 2011
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 JIRA issue and include the issue 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, JIRA 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 filename matches one-to-one the filenames in ITK, plus the extension .txt.
- 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.
