Reviewed code should pass all of this checklist. This is intended as a summary of the policy extensively described in the LaunchpadHackingFAQ, Python style guide, TestsStyleGuide.

Configuration

• Will this configuration change affect production machines?
• Do you need to talk to IS about this configuration change?
• What does this change assume about the network?
• What network conditions could cause the change to break?
• What host will the change be running on?

Style

• Is the code layout PEP8 compliant?

• Is the format of multi-line docstrings PEP257 compliant?

• Are the properties named_like_properties, the functions named_like_functions and the methods namedLikeMethods?

• Do if:/elif: blocks have a matching else: clause?

• Does the methods use super() to chain up to their parent?

• Are operator.attrgetter and operator.itemgetter used instead of lambda?

• Do all added methods and classes have an appropriate docstring?
• Do all asserts have an appropriate failure message?

User interface

• Do the templates conform to the UserInterfaceChecklist?

• Is the code added to browser modules appropriate, or would it be more reusable and easier to test in the content class?

• Are there tests for new pages, or changes in behaviour in existing pages?
• If the code is changing GET form arguments, will we get OOPSes because of bookmarked URLs?

• Does JavaScript code conform to our JavaScriptStyleGuide?

• If JavaScript has been added or changed, is there a useful fallback when JavaScript is not available?

• If the code adds/changes JavaScript or CSS, has it been verified in multiple browsers?

• Is the user interface text and help text, on pages being changed, up to date with those changes?
• Are the UI text and/or help changes significant enough to require design review?

Content-classes, interfaces, and SQL

• Does each database table map to a content class in lib/lp/*/model? Is the content class in its own file, named by the table (lower case)?

• Are all enums in the same interface files as the schemas where they are expressed? E.g. TeamMembershipStatus should be in interfaces/teammembership.py (or enums.py), not in interfaces/person.py.

• Have security.py permissions been set up correctly for new content classes? (maybe security.py and zcml)

• Does it use regular permissions as defined in LaunchpadSecurityPolicy?

• Are there tests for API changes or additions in the content class?
• Does it use SQLObject or raw SQL when it could use Storm instead?
• Are multiline SQL triple-quoted?

• If the code is SQL fetching a single object or None, does it use a selectOne*() (SQLObject) or .one() (Storm) variant?

• When using raw SQL, are quote() and sqlvalues() used to interpolate data safely?

• Is the content-classes code free from references to ILaunchpadBag and check_permission()?

• If the branch descends from a DB patch, new sample data may need to be generated. Specifically, this occurs when a new DB field can be NULL and defaults to NULL, meaning that sampledata doesn't need to be updated to be valid. Sampledata changes must be made before the branch is submitted to PQM.

• If the branch contains database classes that uses any non-database attributes, these classes must provide a __storm_invalidated__ method resetting these attributes. (That way it can be reused safely across transactions)

View Classes

• Are values offered using properties and attributes, rather than no-arg methods?

Gotchas

• Use of the built-in hasattr function should be avoided since it swallows exceptions. Use the safe_hasattr() function from lazr.restful:

    from lazr.restful.utils import safe_hasattr

• Does the branch change the assumptions of the code about the network environment it will be running it (does it now connect directly to internet hosts or does it connect to some specific internal server?). Have these changes been run past IS?

Tests

• Do the doctests added to lib/lp/*/doc read well as documentation? Or would they be more appropriate in other non-documentation directories?

• Do the added page tests read well as use-cases?

• Is each new interface defined appropriately documented and has a verifyObject() test?

• Can the overhead of the new tests setUp/tearDown fixture be shared with other tests through the use of a layer?

• Are all global changes (e.g. globals, sys.path or lazr.config modifications, calls to socket.setdefaulttimeout()) done by the test reverted once the test completed (successfully or not)?

• If the test fails, will the failure message be helpful in diagnosing the problem (e.g. the test avoids comparison to True or False when checking for the presence of a string)?

• Do the page tests use the appropriate BeautifulSoup helpers to extract the relevant HTML section for the test?

• If the test calls a subprocess that writes to the database, does it call DatabaseLayer.force_dirty_database() to ensure that the testing database gets rebuilt for the next test (commits in subprocesses are not detected)?