This page last changed on Sep 10, 2009 by jdeolive.

Motivation

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

Proposal

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?
#. What changes require review?
#. Who is qualified to review code?
#. What are the standards by which a change is judged?
#. What tools are available to streamline the review workflow?

When is a change reviewed?

Option 1: Pre-commit

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.

Option 2: Post-commit

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.

Option 3: Hybrid

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
#. number of classes the change affects
#. wether the change is to a core module or an extension/community module

*Please provide more for this list*

What changes require review?

Option 1: Every change is reviewed

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.

Option 2: Only non-trivial changes are reviewed

The upside here is a reduction in the global burden, as many changes are trivial. The downside here is the ambiguity of "trivial".

Who reviews what?

Option 1: Module maintainers

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.

Option 2: Open

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.

Code Quality

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:

  • Performance
  • Robustness
  • Aesthetics and cleanliness
  • Consistency

Turning this into a list of guidelines would look something like:

  • Code changes should not lead to any performance degradations. Unless of course the change in question is mindfully and willingly sacrificing performance to achieve some goal or feature.
  • All code changes should be robust. Edge and failure cases should always be accounted for.
  • Code aesthetics and cleanliness lead to better maintainable code and should always be kept in mind.
  • Consistency is important. Every programmer has their own style with regard to naming, class design etc.... When coming up with a new class or module a programmer can feel free to come up with class names, but when working in an existing module or class a programmer should delegate to the surrounding class/module following conventions already put in place.

TODO: this list needs to be expanded.

Tools

Document generated by Confluence on May 14, 2014 23:00