It seemed sensible to me to come up with a default IMP code review policy. Please take a look over it and if you have any issues with it, think it needs to be clarified some more, etc., follow up to the list: https://salilab.org/internal/manuals/imp/ch01s04.html
Text from that URL:
Code review
To ensure code quality, additions to the IMP kernel should be reviewed by the other developers. In general, this means posting a patch with some explanation of the changes (e.g. suitable for the source control logs) to the imp-dev@salilab.org mailing list. This is mandatory for changes that:
* Change the API (e.g. remove or rename existing methods or classes, change the arguments taken by existing methods, change the behavior of existing methods, change the return values or thrown exceptions of existing methods).
* Remove or modify existing test cases.
For more minor changes, such as adding new tests, methods and/or classes (which may change the ABI but not the API), documentation updates, etc. review is not required but is recommended.
Patches should contain a related set of changes. For example, a patch which adds a new method foo, a new testcase for foo, and some documentation for the SpecialVector class, should be split into two patches: one for the foo method and test, and the other for the SpecialVector documentation.
New code in patches should also adhere to the coding style guidelines.
Rationale: Other developers may have external applications and their own in-progress work which may be adversely affected by your changes. There may also be disagreement about a change, so discussing it on the mailing list first assures that a consensus is first reached, rather than other developers later reverting the change. It is much easier to review a patch if it is small and contains only relevant changes, and in addition it can generally be applied to source control virtually unchanged if the code is OK.
Ben
certainly makes sense to me. thanks, andrej
On Dec 12, 2007, at 12:50 PM, Ben Webb wrote:
> It seemed sensible to me to come up with a default IMP code review > policy. Please take a look over it and if you have any issues with it, > think it needs to be clarified some more, etc., follow up to the list: > https://salilab.org/internal/manuals/imp/ch01s04.html > > Text from that URL: > > Code review > > To ensure code quality, additions to the IMP kernel should be reviewed > by the other developers. In general, this means posting a patch with > some explanation of the changes (e.g. suitable for the source control > logs) to the imp-dev@salilab.org mailing list. This is mandatory for > changes that: > > * Change the API (e.g. remove or rename existing methods or classes, > change the arguments taken by existing methods, change the behavior of > existing methods, change the return values or thrown exceptions of > existing methods). > > * Remove or modify existing test cases. > > For more minor changes, such as adding new tests, methods and/or > classes > (which may change the ABI but not the API), documentation updates, > etc. > review is not required but is recommended. > > Patches should contain a related set of changes. For example, a patch > which adds a new method foo, a new testcase for foo, and some > documentation for the SpecialVector class, should be split into two > patches: one for the foo method and test, and the other for the > SpecialVector documentation. > > New code in patches should also adhere to the coding style guidelines. > > Rationale: Other developers may have external applications and their > own > in-progress work which may be adversely affected by your changes. > There > may also be disagreement about a change, so discussing it on the > mailing > list first assures that a consensus is first reached, rather than > other > developers later reverting the change. It is much easier to review a > patch if it is small and contains only relevant changes, and in > addition > it can generally be applied to source control virtually unchanged if > the > code is OK. > > Ben > -- > ben@salilab.org http://salilab.org/~ben/ > "It is a capital mistake to theorize before one has data." > - Sir Arthur Conan Doyle > _______________________________________________ > IMP-dev mailing list > IMP-dev@salilab.org > https://salilab.org/mailman/listinfo/imp-dev
-- Andrej Sali, Ph.D. Professor and Vice Chair, Department of Biopharmaceutical Sciences Department of Pharmaceutical Chemistry California Institute for Quantitative Biosciences University of California at San Francisco UCSF MC 2552 Byers Hall Room 503B 1700 4th Street San Francisco, CA 94158-2330, USA Tel +1 (415) 514-4227; Fax +1 (415) 514-4231 Assistant, Ms. Karin Asensio, Tel +1 (415)514-4228; Lab +1 (415) 514-4232, 4233, 4258 Email sali@salilab.org; Web http://salilab.org
participants (2)
-
Andrej Sali -
Ben Webb