launchpad-reviewers team mailing list archive

[Colin Watson <mp+411686@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:dev-guides into launchpad:master.

Commit message:
Add some guides from dev.launchpad.net


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/411686

These were previously:

  https://dev.launchpad.net/ArchitectureGuide
  https://dev.launchpad.net/PythonStyleGuide

I've kept the material essentially unchanged, except for converting to reStructuredText formatting and making the occasional change to a more documentation-style voice (e.g. avoiding first-person language).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:dev-guides into launchpad:master.

diff --git a/doc/guides/architecture.rst b/doc/guides/architecture.rst
new file mode 100644
index 0000000..239b7c8
--- /dev/null
+++ b/doc/guides/architecture.rst
@@ -0,0 +1,235 @@
+===================
+Architectural Guide
+===================
+
+All the code we write will meet these values to a greater or lesser degree.
+Where you can, please make choices that make you code more strongly meet
+these values.
+
+Some existing code does not meet them well; this is simply an opportunity to
+get big improvements - by increasing e.g. the transparency of existing code,
+operational issues and debugging headaches can be reduced without a great
+deal of work.
+
+This guide is intended as a living resource: all Launchpad developers, and
+other interested parties, are welcome to join in and improve it.
+
+Goals
+=====
+
+The goal of the recommendations and suggestions in this guide are to help us
+reach a number of big picture goals.  We want Launchpad to be:
+
+ * Blazingly fast
+ * Always available
+ * Change safely
+ * Simple to make, manage and use
+ * Flexible
+
+However it's hard when making any particular design choice to be confident
+that it drives us towards these goals: they are quite specific, and not
+directly related to code structure or quality.
+
+Related documents
+-----------------
+
+Launchpad is moving to a `services-based design
+<https://dev.launchpad.net/ArchitectureGuide/Services>`_.  This is intended
+to faciliate achieving many of the goals from this guide.
+
+The :doc:`Python style guide <python>` specifies coding style guidelines.
+
+Values
+======
+
+There are a number of things that are more closely related to code, which do
+help drive us towards our goals.  The more our code meets these values, the
+easier it will be to meet our goals.
+
+The values are:
+
+ * Transparency
+ * Loose coupling
+ * Highly cohesive
+ * Testable
+ * Predictable
+ * Simple
+
+Transparency
+------------
+
+Transparency speaks to the ability to analyse the system without dropping
+into ``pdb`` or taking guesses.
+
+Some specific things that aid transparency:
+
+ * For blocking calls (SQL, bzr, email, librarian, backend web services,
+   memcache) use ``lp.services.timeline.requesttimeline`` to record the
+   call.  This includes it in OOPS reports.
+ * fine grained just-in-time logging (e.g. bzr's ``-Dhpssdetail`` option)
+
+ * live status information 
+   * (+opstats, but more so)
+   * cron script status
+   * migration script status
+
+ * regular log files
+ * OOPS reports - lovely
+ * Which revisions/versions of the software & its dependencies are running
+
+We already have a lot of transparency.  We can use more.
+
+Aim for automation, developer usability, minimal SRE intervention, on-demand
+access.
+
+When adding code to the system, ask yourself "how will I troubleshoot this
+when it goes wrong without access to the machine it is running on".
+
+Loose coupling
+--------------
+
+The looser the coupling between different parts of the system the easier it
+is to change them.  Launchpad is pretty good about this in some ways due to
+the component architecture, but it's not the complete story: decreasing the
+coupling more will help the system.  Consider examples such as the jobs
+system and the build farm.
+
+The acid test for the coupling of a component is "how hard is it to reuse?".
+
+Of particular note, many changes in one area of the system (e.g. bugs) break
+tests in other areas (e.g. blueprints) - this adds a lot of developer
+friction and is a strong sign of overly tight coupling.
+
+Highly cohesive
+---------------
+
+The more things a component does, the harder it is to reason about it and
+performance-tune it.  This is "Do one thing well" in another setting.
+
+A good way to assess this is to look inside the component and see if it is
+doing one thing, or many things.
+
+One common sign for a problem in this area is attributes (or persistent
+data) that are not used in many methods - that often indicates there is a
+separate component embedded in this one.
+
+There are tradeoffs here due to database efficiency and normalisation, but
+it's still worth thinking about: narrower tables can perform better and use
+less memory, even if we do add extra tables to support them.
+
+On a related note, the more clients using a given component, the wider its
+responsibilities and the more critical it becomes.  That's an easy situation
+to end up with too much in one component (lots of clients wanting things
+decreases the cohesion), and then we have a large unwieldy critical
+component - not an ideal situation.
+
+Testable
+--------
+
+We write lots of unit and integration tests at the moment.  However it's not
+always easy to test specific components - and the coupling of the components
+drives this.
+
+The looser the coupling, the better in terms of having a very testable
+system.  However loose coupling isn't enough on its own, so we should
+consider testability from a few angles:
+
+Can it be tested in isolation? If it can, it can be tested more easily by
+developers and locally without needing lots of testbed environment every
+time.
+
+Can we load test it? Not everything needs this, but if we can't load test a
+component that we reasonably expect to see a lot of use, we may have
+unpleasant surprises down the track.
+
+Can we test it with broken backends/broken data? It is very nice to be
+confident that when a dependency breaks (not if) the component will behave
+nicely.
+
+It's also good to make sure that someone else maintaining the component
+later can repeat these tests and is able to assess the impact of their
+changes.
+
+Automation of this stuff rocks!
+
+Predictable
+-----------
+
+An extension of stability - servers should stay up, database load should be
+what it was yesterday, rollouts should move metrics in an expected
+direction.
+
+Predictability is pedestrian, but it's very useful: useful for capacity
+planning, useful for changing safely, useful for being highly available, and
+useful for letting us get on and do new/better things.
+
+The closer to a steady state we can get, the more obvious it is when
+something is wrong.
+
+Simple
+------
+
+A design that allows for future growth is valuable, but it is not always
+clear how much growth to expect, or in the case of code extension, what
+kind.  In this case, it is better to design the simplest thing that will
+work at the time being, and update the design when you have a better idea of
+what's needed.  Simplicity also aids comprehension and reduces the surface
+area for bugs to occur.
+
+Related ideas are KISS, YAGNI, and avoiding premature optimization, but it
+is always important to apply judgement.  For example, avoiding premature
+optimization does not justify rolling your own inefficient sort function.
+
+Make the design as simple as possible, but no simpler.  Note that simple
+does not mean simplistic.
+
+Performance
+===========
+
+Document how components are expected to perform.  Docstrings are great
+places to put this.  E.g. "This component is expected to deal with < 100 bug
+tracker types; if we have more this will need to be redesigned.", or "This
+component compares two user accounts in a reasonable time, but when
+comparing three or more it's unusable."
+
+Try to be concrete.  For instance: "This component is O(N) in the number of
+bug tasks associated with a bug." is OK, but better would be "This component
+takes 40ms per bug task associated with a bug."
+
+Testing
+=======
+
+Tests for a class should complete in under 2 seconds.  If they aren't, spend
+at least a little time determining why.
+
+Transparency
+============
+
+Behaviour of components should be analysable in lpnet without asking SREs:
+that is, if a sysadmin is needed to determine what's wrong with something,
+we've designed it wrong.  Let's make sure there is a bug for that particular
+case, or if possible Just Fix It.
+
+Emit Python logging messages at an appropriate importance level: warning or
+error for things operators need to know about, info or debug for things that
+don't normally indicate a problem.
+
+Coupling
+========
+
+No more than 5 dependencies of a component.
+
+Cohesion
+========
+
+Attributes should be used in more than 1/3 of interactions.  If they are
+used less often than that, consider deleting or splitting into separate
+components.
+
+If you can split one class into two or more that would individually make
+sense and be simpler, do it.
+
+..
+    The ideas in this document are open to discussion and change.  If you
+    feel strongly about an issue, make a merge proposal with your
+    suggestions.
diff --git a/doc/guides/python.rst b/doc/guides/python.rst
new file mode 100644
index 0000000..3ceb3ad
--- /dev/null
+++ b/doc/guides/python.rst
@@ -0,0 +1,471 @@
+==================
+Python Style Guide
+==================
+
+This document describes expected practices when writing Python code.  There
+are occasions when you can break these rules, but be prepared to justify
+doing so when your code gets reviewed.
+
+Existing Conventions
+====================
+
+There are well-established conventions in the Python community, and in
+general we should follow these.  General Python conventions, and required
+reading:
+
+ * `PEP 8 <https://www.python.org/dev/peps/pep-0008/>`_: Style Guide for
+   Python Code
+ * `PEP 257 <https://www.python.org/dev/peps/pep-0257/>`_: Docstring
+   Conventions
+ * The Zen of Python: ``python3 -c "import this"``
+
+Note that our standards differ slightly from PEP-8 in some cases.
+
+Coding standards other projects use:
+
+ * `Twisted Coding Standard
+   <https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html>`_
+ * `Zope developer guidelines
+   <https://www.zope.org/developer/guidelines.html>`_
+
+Related Documents
+=================
+
+ * `Exception guidelines <https://dev.launchpad.net/ExceptionGuidelines>`_
+ * `Assertions <https://dev.launchpad.net/AssertionsInLaunchpad>`_
+ * `Launchpad hacking FAQ <https://dev.launchpad.net/LaunchpadHackingFAQ>`_
+ * `Tests style guide <https://dev.launchpad.net/TestsStyleGuide>`_
+
+Whitespace and Wrapping
+=======================
+
+(Most of this is likely to be delegated to `black
+<https://github.com/psf/black>`_ in the near future.)
+
+ * Code should fit within 78 columns, so as to fit nicely in an 80 column
+   terminal, even when quoted in an email.
+ * Indents should be 4 spaces.
+ * **No tabs**.  This is not negotiable.
+
+.. _multiline:
+
+Multiline braces
+----------------
+
+There are lots of cases where you might have a list, tuple or dictionary
+literal that spans multiple lines.  In these cases, you should consider
+formatting these literals as follows.  This format makes changes to the list
+clearer to read in a diff.  Note the trailing comma on the last element.
+
+.. code-block:: python
+
+    mydict = {
+        'first': 1,
+        'second': 2,
+        'third': 3,
+        }
+
+    mylist = [
+        'this is the first line',
+        'this is the second line',
+        'this is the third line',
+        ]
+
+Naming
+======
+
+Consistency with existing code is the top priority.  We follow PEP-8 with
+the following exceptions:
+
+ * ``CamelCase``: classes, interfaces (beginning with ``I``)
+ * ``initialCamelCase``: methods
+ * ``lowercase_underscores``: functions, non-method attributes, properties,
+   local variables
+ * ``ALL_CAPS``: constants
+
+Private names are private
+-------------------------
+
+You should never call a non-public attribute or method from another class.
+In other words, if class A has a method ``_foo()``, don't call it from
+anywhere outside class A.
+
+Docstrings
+==========
+
+ * If you haven't already, read `PEP 257
+   <https://www.python.org/dev/peps/pep-0257/>`_.
+ * In general, everything that can have a docstring should: modules,
+   classes, methods, functions.
+ * Docstrings should always be enclosed in triple double quotes: ``"""Like
+   this."""``
+ * When a class or a method implements an interface, the docstring should
+   say ``"""See `IFoo`."""``
+
+Docstrings should be valid `reStructuredText
+<https://docutils.sourceforge.io/rst.html>`_ (with all the painful
+indentation rules that implies) so that tools such as `pydoctor
+<https://pypi.org/project/pydoctor/>`_ can be used to automatically generate
+API documentation.
+
+You should use field names as defined in the `epydoc
+<http://epydoc.sourceforge.net/fields.html>`_ documentation but with reST
+syntax.
+
+Using \`name\` outputs a link to the documentation of the named object, if
+pydoctor can figure out what it is.
+
+Here is a comprehensive example.  Parameter descriptions are a good idea but
+not mandatory.  Describe in as much or as little detail as necessary.
+
+.. code-block:: python
+
+    def example2(a, b):
+        """Perform some calculation.
+
+        It is a **very** complicated calculation.
+
+        :param a: The number of gadget you think this
+                  function should frobnozzle.
+        :type a: ``int``
+        :param b: The name of the thing.
+        :type b: ``str``
+        :return: The answer!
+        :rtype: ``str``.
+        :raise ZeroDivisionError: when ``a`` is 0.
+        """
+
+Modules
+=======
+
+Each module should look like this:
+
+.. code-block:: python
+
+    # Copyright 2009-2011 Canonical Ltd.  All rights reserved.
+
+    """Module docstring goes here."""
+
+    __all__ = [
+        ...
+        ]
+
+The file ``standard_template.py`` has most of this already, so save yourself
+time by copying that when starting a new module.  The "..." should be filled
+in with a list of public names in the module.
+
+PEP-8 says to put any relevant ``__all__`` specifications after the module
+docstring but before any import statements (except for ``__future__``
+imports, which in most cases we no longer use).  This makes it easy to see
+what a module contains and exports, and avoids the problem that differing
+amounts of imports among files means that the ``__all__`` list is in a
+different place each time.
+
+.. _imports:
+
+Imports
+=======
+
+Restrictions
+------------
+
+There are restrictions on which imports can happen in Launchpad.  Namely:
+
+ * View code cannot import code from ``lp.*.model``.
+ * ``import *`` cannot be used if the module being imported from does not
+   have an ``__all__``.
+ * Database code may not import ``zope.exceptions.NotFoundError`` -- it must
+   instead use ``lp.app.errors.NotFoundError``.
+
+These restrictions are enforced by the Import Pedant, which will cause your
+tests not to pass if you don't abide by the rules.
+
+Use absolute imports (``from foo.bar import Bar``), not relative imports
+(``from .bar import Bar``).
+
+Multiline imports
+-----------------
+
+You should be using Launchpad's default `pre-commit
+<https://dev.launchpad.net/Running#pre-commit>`_ setup, which automatically
+formats your imports using ``isort`` before you commit.  The remainder of
+this section is for information.
+
+Sometimes import lines must span multiple lines, either because the package
+path is very long or because there are multiple names inside the module that
+you want to import.
+
+**Never use backslashes in import statements!**  Use parenthesized imports:
+
+.. code-block:: python
+
+    from foo import (
+        That, 
+        TheOther, 
+        This,
+        )
+
+Like other lists, imports should list one item per line.  The exception is
+if only one symbol is being imported from a given module.
+
+.. code-block:: python
+
+    from lp.app.widgets.itemswidgets import CheckBoxMatrixWidget
+
+But if you import two or more, then each item needs to be on a line by
+itself.  Note the trailing comma on the last import and that the closing
+paren is on a line by itself.
+
+.. code-block:: python
+
+    from lp.app.widgets.itemswidgets import (
+        CheckBoxMatrixWidget,
+        LaunchpadRadioWidget,
+        )
+
+Import scope
+------------
+
+We encourage importing names from the location they are defined in.  This
+seems to work better with large complex components.
+
+Circular imports
+----------------
+
+With the increased use of native Storm APIs, you may encounter more circular
+import situations.  For example, a ``MailingList`` method may need a
+reference to the ``EmailAddress`` class for a query, and vice versa.  The
+classic way to solve this is to put one of the imports inside a method
+instead of at module global scope (a "nested import").
+
+Short of adopting something like Zope's lazy imports (which has issues of
+its own), you can't avoid this, so here are some tips to make it less
+painful.
+
+ * Do the nested import in the least common case.  For example, if 5 methods
+   in ``model/mailinglist.py`` need access to ``EmailAddress`` but only one
+   method in ``model/emailaddress.py`` needs access to ``MailingList``, put
+   the import inside the ``emailaddress.py`` method, so you have fewer
+   overall nested imports.
+ * Clearly comment that the nested import is for avoiding a circular import,
+   using the example below.
+ * Put the nested import at the top of the method.
+
+... code-block:: python
+
+    def doFooWithBar(self, ...):
+        # Import this here to avoid circular imports.
+        from lp.registry.model.bar import Bar
+        # ...
+        return store.find((Foo, Bar), ...)
+
+Circular imports and webservice exports
+---------------------------------------
+
+One of the largest sources of pain from circular imports is caused when you
+need to export an interface on the webservice.  Generally, the only way
+around this is to specify generic types (like the plain old ``Interface``)
+at declaration time and then later patch the webservice's data structures at
+the bottom of the interface file.
+
+Fortunately there are some helper functions to make this less painful, in
+``lib/lp/services/webservice/apihelpers.py``.  These are simple functions
+where you can give some info about your exported class/method/parameters and
+they do the rest for you.
+
+For example:
+
+.. code-block:: python
+
+    from lp.services.webservice.apihelpers import (
+        patch_entry_return_type,
+        patch_collection_return_type,
+        )
+    patch_collection_return_type(
+        IArchive, 'getComponentsForQueueAdmin', IArchivePermission)
+    patch_entry_return_type(
+        IArchive, 'newPackageUploader', IArchivePermission)
+
+Properties
+==========
+
+Properties are expected to be cheap operations.  It is surprising if a
+property is not a cheap operation.  For expensive operations use a method,
+usually named ``getFoo()``.  Using ``cachedproperty`` provides a work-around
+but it should not be overused.
+
+Truth conditionals
+==================
+
+Remember that False, None, [], and 0 are not the same although they all
+evaluate to False in a boolean context.  If this matters in your code, be
+sure to check explicitly for either of them.
+
+Also, checking the length may be an expensive operation.  Casting to bool
+may avoid this if the object specializes by implementing ``__bool__``.
+
+Chaining method calls
+=====================
+
+Since in some cases (e.g. class methods and other objects that rely on
+descriptor ``__get__()`` behaviour) it's not possible to use the old style
+of chaining method calls (``SuperClass.method(self, ...))``, we should
+always use the ``super()`` builtin when we want that. 
+
+.. note::
+
+    The exception to this rule is when we have class hierarchies outside of
+    our control that are known not to use ``super()`` and that we want to
+    use for diamond-shaped inheritance.
+
+Use of lambda, and operator.attrgetter
+======================================
+
+Prefer `operator.attrgetter
+<https://docs.python.org/3/library/operator.html#operator.attrgetter>`_ to
+``lambda``.  Remember that giving functions names makes the code that calls,
+passes and returns them easier to debug.
+
+Use of hasattr
+==============
+
+Use ``safe_hasattr`` from ``lazr.restful.utils`` instead of the built-in
+``hasattr`` function because the latter swallows exceptions.
+
+Database-related
+================
+
+Storm
+-----
+
+We use two database ORM (object-relational mapper) APIs in Launchpad, the
+older and deprecated SQLObject API and the new and improved `Storm
+<https://storm.canonical.com>`_ API.  All new code should use the Storm API,
+and you are encouraged to convert existing code to Storm as part of your
+tech-debt payments.
+
+.. note::
+
+    The SQLObject and Storm ``ResultSet`` interfaces are not compatible, so
+    e.g. if you need to ``UNION`` between these two, you will run into
+    trouble.  We are looking into ways to address this.
+
+Field attributes
+----------------
+
+When you need to add ID attributes to your database class, use ``field_id``
+as the attribute name instead of ``fieldID``.
+
+Multi-line SQL
+--------------
+
+SQL doesn't care about whitespace, so use triple quotes for large SQL
+queries or fragments, e.g.:
+
+.. code-block:: python
+
+    query = """
+        SELECT TeamParticipation.team, Person.name, Person.displayname
+        FROM TeamParticipation
+        INNER JOIN Person ON TeamParticipation.team = Person.id
+        WHERE TeamParticipation.person = %s
+        """ % sqlvalues(personID)
+
+This is also easy to cut-and-paste into ``psql`` for interactive testing,
+unlike if you use several lines of single quoted strings.
+
+Creating temporary files
+========================
+
+We should use the most convenient method of the ``tempfile`` module.  Never
+taint ``/tmp/`` or any other "supposed to be there" path.
+
+Despite being developed and deployed on Ubuntu systems, turning it into a
+restriction might not be a good idea.
+
+When using ``tempfile.mkstemp`` remember it returns an open file descriptor
+which has to be closed or bound to the open file, otherwise they will leak
+and eventually hit the default Linux limit (1024).
+
+There are two good variations according to the scope of the temporary file.
+
+.. code-block:: python
+
+    fd, filename = mkstemp()
+    os.close(fd)
+    ...
+    act_on_filename(filename)
+
+Or:
+
+.. code-block:: python
+
+    fd, filename = mkstemp()
+    with os.fdopen(fd, 'w') as temp_file:
+        ...
+        temp_file.write('foo')
+
+**Never** use:
+
+.. code-block:: python
+
+    fd, filename = mkstemp()
+    with open(filename) as temp_file:
+        temp_file.write('foo')
+    # BOOM! 'fd' leaked.
+
+In tests, you should use the ``TempDir`` fixture instead, which cleans
+itself up automatically:
+
+.. code-block:: python
+
+    from fixtures import TempDir
+
+    class TestFoo(TestCase):
+    ...
+        def test_foo(self):
+            tempdir = self.useFixture(TempDir).path
+            ...
+            do_something(os.path.join(tempdir, 'test.log'))
+            ...
+
+Configuration hints
+===================
+
+Vim
+---
+
+To make wrapping and tabs fit the above standard, you can add the following
+to your ``.vimrc``:
+
+.. code-block:: vim
+
+    autocmd BufNewFile,BufRead *.py set tw=78 ts=4 sts=4 sw=4 et
+
+To make trailing whitespace visible:
+
+.. code-block:: vim
+
+    set list
+    set listchars=tab:>.,trail:-
+
+This will also make it obvious if you accidentally introduce a tab.
+
+To make long lines show up:
+
+.. code-block:: vim
+
+    match Error /\%>79v.\+/
+
+For an even more in-depth Vim configuration, have a look at
+`UltimateVimPythonSetup <https://dev.launchpad.net/UltimateVimPythonSetup>`_
+for a complete vim file you can copy to your local setup.
+
+Emacs
+-----
+
+There are actually two Emacs Python modes.  Emacs comes with ``python.el``
+which has some quirks and does not seem to be as popular among hardcore
+Python programmers.  `python-mode.el <https://launchpad.net/python-mode>`_
+comes with XEmacs and is supported by a group of hardcore Python
+programmers.  Even though it's an add-on, it works with Emacs just fine.
diff --git a/doc/index.rst b/doc/index.rst
index c9eed9a..b074f58 100644
--- a/doc/index.rst
+++ b/doc/index.rst
@@ -21,6 +21,15 @@ Overview
    scope
    values
 
+Guides
+======
+
+.. toctree::
+   :maxdepth: 1
+
+   guides/architecture
+   guides/python
+
 Technical
 =========