Diff for "CodeReviewChecklist"

Not logged in - Log In / Register

Differences between revisions 1 and 9 (spanning 8 versions)
Revision 1 as of 2009-07-15 20:16:03
Size: 5167
Editor: flacoste
Comment:
Revision 9 as of 2020-01-06 17:22:46
Size: 5395
Editor: cjwatson
Comment: mention Storm
Deletions are marked like this. Additions are marked like this.
Line 9: Line 9:
 * Do you need to talk to the OSAs about this configuration change?  * Do you need to talk to IS about this configuration change?
Line 19: Line 19:
 * Is the code layout [[http://www.python.org/dev/peps/pep-0008/|PEP8]] compliant?  * Is the code layout [[https://www.python.org/dev/peps/pep-0008/|PEP8]] compliant?
Line 21: Line 21:
 * Is the format of multi-line docstrings [[http://www.python.org/dev/peps/pep-0257/|PEP257]] compliant?  * Is the format of multi-line docstrings [[https://www.python.org/dev/peps/pep-0257/|PEP257]] compliant?
Line 49: Line 49:
 * If the code adds/changes Java``Script or CSS, has it been verified (''e.g.'' by Diogo) in multiple browsers?  * If the code adds/changes Java``Script or CSS, has it been verified in multiple browsers?
Line 53: Line 53:
 * Are the UI text and/or help changes significant enough to require design review?
Line 55: Line 57:
 * Does each database table map to a content class in `lib/canonical/launchpad/database`? Is the content class in its own file, named by the table (lower case)?  * 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)?
Line 57: Line 59:
 * Are all enums in the same interface files as the schemas where they are expressed? E.g. `TeamMembershipStatus` should be in `interfaces/teammembership.py`, not in `interfaces/person.py`.  * 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`.
Line 61: Line 63:
 * Does it use regular permissions as defined in LaunchpadSecurityPolicy?
Line 62: Line 66:

 * Does it use SQLObject or raw SQL when it could use Storm instead?
Line 65: Line 71:
 * If the code is SQL fetching a single object or None, does it use a `selectOne*()` variant?  * If the code is SQL fetching a single object or None, does it use a `selectOne*()` (SQLObject) or `.one()` (Storm) variant?
Line 67: Line 73:
 * Is `quote()` and `sqlvalues()` use to interpolate safely data in the SQL statements?  * When using raw SQL, are `quote()` and `sqlvalues()` used to interpolate data safely?
Line 81: Line 87:
 * Use of the built-in `hasattr` function should be avoided since it swallows exceptions. Use the `safe_hasattr()` function from `lazr`:  * Use of the built-in `hasattr` function should be avoided since it swallows exceptions. Use the `safe_hasattr()` function from `lazr.restful`:
Line 83: Line 89:
    from canonical.lazr.utils import safe_hasattr     from lazr.restful.utils import safe_hasattr
Line 90: Line 96:
 * Do the doctests added to `lib/canonical/launchpad/doc` reads well as documentation? Or would they be more appropriate in other non-documentation directories?  * Do the doctests added to `lib/lp/*/doc` read well as documentation? Or would they be more appropriate in other non-documentation directories?
Line 98: Line 104:
 * Are all global changes (e.g. globals, `sys.path` or `canonical.config` modifications, call to `socket.setdefaulttimeout()`) done by the test reverted once the test completed (successfully or not)?  * 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)?
Line 100: Line 106:
 * If the test fail, will the failure message be helpful in diagnosing the problem (e.g. the test avoid comparison to True or False when checking for the presence of a string.)?  * 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)?
Line 104: Line 110:
 * 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).  * 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)?

Reviewed code should pass all of this checklist. This is intended as a summary of the policy extensively described in the LaunchpadHackingFAQ, PythonStyleGuide, 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)?

CodeReviewChecklist (last edited 2021-11-11 10:01:16 by cjwatson)