GeoServer : Review Practices
This page last changed on Sep 10, 2009 by jdeolive.
Over the years certain review practices have emerged in GeoServer. Some areas of the code base have come more prone to review than others, with "psuedo maintainers" forming around certain modules and packages.
Without a formal review process in place a few problems have formed. The first is that developers are unsure about when a commit requires a review. The second is that certain developers appear to be defensive of certain parts of the code base, making the false assumption that the same review practices are followed by everyone.
This proposal aims to remove this ambiguity and formalize GeoServer review practices
Code review practices change from project to project. But in general projects follow either a pre-commit or a post-commit system. In a pre-commit system code under review must be reviewed and accepted before it is committed into svn. Contrastingly a post-commit system allows code to be reviewed after it is committed into svn, and any changes required are made as subsequent commits.
There is also the question of what changes need to be reviewed, and what changes do not, which also varies from project to project. Some projects draw a hard line and state that every single line, no matter how trivial, must be reviewed. Other projects allow a bit of leeway in that trivial changes can pass without review.
And finally is the question of who reviews what? Some projects have module ownership, which makes the decision clear. However other projects (like GeoServer) have an more open policy.
In this proposal a few different options are presented to answer the following questions:
#. When is a change reviewed?
The upside with this option is that code is reviewed and meets quality standard before it is committed into the repository. This alleviates the problem of a review being omitted due to being put off by a developer who is busy and eventually put off for so long that it never gets done.
The downside here is that it reduces the productivity of the developer making the change as they can not commit straight away, but have to wait for a successful review.
The upside/downside here is simply opposite of the pre-commit option. However it could also be argued that a post-commit system decreases productivity in that it forces the developer to be more careful about what they are committing, and do more "self-review". Unless the developer is sloppy in which case a post-commit system would increase the amount of sloppy code being committed. Either way its a downside.
A hybrid system could be adopted in which some changes are reviewed pre-commit and some changes reviewed post-commit. The question then becomes what changes require a pre vs post commit review. One answer could be "trivial" changes are reviewed post-commit, and all others reviewed pre-commit.
A problem with this solution is ambiguity as the definition of trivial differs depends on who you ask. What one developer may consider trivial another developer may not. However it might be possible to come up with a non-ambigious definition of "trivial". Some possibilities could be to classify a change as trivial by:
#. number of lines of code
*Please provide more for this list*
The upside of this approach s that it is totally unambiguous.
The downside is that reviewing a change reduces productivity among the developer who has to wait for the change to be reviewed, and the developers who have to take the time to review the change.
The upside here is a reduction in the global burden, as many changes are trivial. The downside here is the ambiguity of "trivial".
The first option would be to formally assign module maintainers to GeoServer modules. With module maintainers in place a change must be reviewed by at least one of the maintainers of each module the change affects.
The second option would be to keep an open system in which any committer is free to review on any change. This has the potential to lead to problems in which a developer has an issue with a change that might not be agreed upon by other reviewers. It then becomes ambiguous wether or not to block the change or not.
Currently there are no official code quality standards by which code is reviewed. Every developer tends to look for the same thing. If coming up with a formal review process these quality guidelines will need to be codified somehow.
There are lots of "QA checklists" available which could probably serve as a baseline. However every project is unique and has its own specific things that define quality. In past GeoServer code review the things people tend to comment on (apart from the obvious) are:
Turning this into a list of guidelines would look something like:
TODO: this list needs to be expanded.
|Document generated by Confluence on May 14, 2014 23:00|