wiki:CodeReview

At a minimum, a patch should:

  1. pass the entire test suite reliably
  2. pass 'make pyflakes'
  3. include documentation (in master/docs) for any new or changed functionality
  4. be licensed under the GPLv2 (this is assumed for any contribution; see LicensingYourContribution)
  5. be compatible with Python-2.5

Tests

In general, all new functionality should have tests. However, at the moment some parts of Buildbot have very few tests and are difficult to write tests for, so this is not a hard-and-fast rule.

Docstrings

Buildbot has had an inconsistent history with respect to docstrings. We now follow a policy of documenting something once. Thus, if a class is intended for use by users (e.g., a new step), it should be documented in the Texinfo documentation (master/docs) and not in a docstring. Conversely, if the changed code is part of an internal Buildbot interface, then it should have a docstring.

Style

Try to adhere to TwistedStyle, and see Buildbot Tests for guidelines on writing tests. Things to avoid:

  • overly long try blocks with a blanket except. These can tend to hide mistakes made when modifying the code.
  • hard-coded values that users might want to adjust
  • difficult-to-follow path of execution. Use nested functions for long deferred chains, rather than methods like _myMethod1, _myMethod2, etc.