Patch Review Process
This document outlines the code review process for GeoNode code. Each commit proposed for inclusion in GeoNode should be reviewed by at least one developer other than the author. For pragmatic reasons, some developers, referred to in this document as core committers, may commit directly to the GeoNode repository without waiting for review. Such changes are still subject to review, and may be reverted if they fail any of the Review Criteria.
A related process is Improvement Proposals. While patch review protects code quality in the GeoNode project at a small granularity, the Improvement Proposal process is intended to promote coordinated design and feedback for larger modifications such as new features or architectural changes.
By requiring a review of code coming into GeoNode, we aim to maintain code quality within the GeoNode project while still allowing contributions from the GeoNode community.
Patch reviews should adhere to the standards set in the Review Criteria, a Project Steering Committee approved set of criteria for the inclusion of new code.
For contributors who do not have commit access to the GeoNode repository, the review process is as follows:
- Find or create a ticket describing the feature or bug to be resolved.
- Create changes to GeoNode code addressing the ticket.
- Publish those changes and request a review from the GeoNode committers. The recommended format is a GitHub pull request. If you are unable or unwilling to provide a change as a branch on GitHub, please consult the developer's list for advice.
- At least one GeoNode committer should review the submitted changes. If he finds the patch acceptable, the changes will be pulled into GeoNode. If problems are found, then he should list them clearly in order to allow the original author to update the submission (at which point we return to point 2 in this process.) In the case of a feature idea which is simply not suitable for inclusion as core GeoNode functionality, the patch may be rejected outright.
The general workflow for code patches coming into GeoNode.
Note: If after a few days your patch has not been reviewed by any GeoNode committer, please feel free to bring it up either in the mailing list or the IRC channel. The GeoNode community (and it's committers) try to respond quickly and give adequate feedback to maintain the interest of new potential members. However, sometimes other responsibilities prevent us from responding quickly.
It is assumed that core committers are familiar with the quality guidelines and capable of producing acceptable patches without the need for waiting on review. Therefore, core committers may make changes without requesting review first (although they are welcome to request review for code if they feel it is appropriate.) For commits made without prior review, committers should review the changes and revert them if they are in violation of the project quality guidelines.
In order for a developer to become a core committer, he must demonstrate familiarity with the quality guidelines for the GeoNode project by producing at least two patches which successfully pass review and are merged without requiring modification. A candidate for core committer-ship must be nominated by a member of the Project Steering Committee, and approved via Apache consensus voting among PSC members.
When a patch is rejected in the Patch Review Process, it should be given a valid review.
This review should point out issues with the patch where it fails to meet one or more of the following criteria.
GeoNode needs to be coherent software despite the diverse interests driving its development. Therefor, major new features need to first be approved according to the Improvement Process.
If a patch fails by this criterion, then its developer is welcome to go through the improvement process to get approval. Otherwise, they can refactor their patch into a GeoNode extension.
We strive to keep GeoNode well-documented. If a patch contributes significant functionality to GeoNode that requires documentation to be understood, the patch review is an opportunity to hold the developer accountable for providing the adequate documentation.
We strive to build GeoNode in a way that can be used in many different localities, by all languages. While there is no localization requirement for GeoNode besides providing default English text, new user-facing features need to be sufficiently internationalized so that others can write translations.
We strive to keep the default user interface for GeoNode appealing for new users and developer's approaching the project. If a patch significantly diminishes the user experience of the software, then a patch may be rejected with a review of how to improve it.
N.B.: Good design can sometimes be in the eye of the beholder. Developer's are encouraged to consult the community and/or a designer about the user interface design of their patches, and to be humble in their design criticisms of others.
To make development easier for others and guarantee software quality, we strive to have good automated test coverage in GeoNode. Patches may fail a review for having insufficient unit and/or integration tests.
Reviews saying that a patch has insufficient tests should offer actionable advice on how to improve those tests. This advice could be to improve code coverage. It may also be a list of possible cases that currently lack tests.
A patch may be rejected for having a known bug, (e.g.) one discovered by reading the code or testing it at the time of review.
New patches should meet GeoNode's code style guidelines. We follow different conventions per language:
- In Java we use the GeoTools/GeoServer convention, essentially the conventions recommended by Oracle modified to make the recommended line length 100 columns instead of 80 to accommodate the long identifiers commonly used in GeoTools code. The GeoServer project provides an Eclipse configuration which helps to stick to this convention.
- In Python we use the conventions enumerated in PEP8. Many editors have plugins available to assist with conformance to this convention.
- In JavaScript we use the OpenLayers conventions, described on the OpenLayers wiki.