~bzr-pqm/bzr/bzr.dev

« back to all changes in this revision

Viewing changes to doc/developers/code-style.txt

  • Committer: Martin Pool
  • Date: 2010-04-01 04:41:18 UTC
  • mto: This revision was merged to the branch mainline in revision 5128.
  • Revision ID: mbp@sourcefrog.net-20100401044118-shyctqc02ob08ngz
ignore .testrepository

Show diffs side-by-side

added added

removed removed

Lines of Context:
1
 
***********************
2
 
Bazaar Code Style Guide
3
 
***********************
4
 
 
5
 
Code layout
6
 
===========
7
 
 
8
 
Please write PEP-8__ compliant code.
9
 
 
10
 
__ http://www.python.org/peps/pep-0008.html
11
 
 
12
 
One often-missed requirement is that the first line of docstrings
13
 
should be a self-contained one-sentence summary.
14
 
 
15
 
We use 4 space indents for blocks, and never use tab characters.  (In vim,
16
 
``set expandtab``.)
17
 
 
18
 
Trailing white space should be avoided, but is allowed.
19
 
You should however not make lots of unrelated white space changes.
20
 
 
21
 
Unix style newlines (LF) are used.
22
 
 
23
 
Each file must have a newline at the end of it.
24
 
 
25
 
Lines should be no more than 79 characters if at all possible.
26
 
Lines that continue a long statement may be indented in either of
27
 
two ways:
28
 
 
29
 
within the parenthesis or other character that opens the block, e.g.::
30
 
 
31
 
    my_long_method(arg1,
32
 
                   arg2,
33
 
                   arg3)
34
 
 
35
 
or indented by four spaces::
36
 
 
37
 
    my_long_method(arg1,
38
 
        arg2,
39
 
        arg3)
40
 
 
41
 
The first is considered clearer by some people; however it can be a bit
42
 
harder to maintain (e.g. when the method name changes), and it does not
43
 
work well if the relevant parenthesis is already far to the right.  Avoid
44
 
this::
45
 
 
46
 
     self.legbone.kneebone.shinbone.toebone.shake_it(one,
47
 
                                                     two,
48
 
                                                     three)
49
 
 
50
 
but rather ::
51
 
 
52
 
     self.legbone.kneebone.shinbone.toebone.shake_it(one,
53
 
         two,
54
 
         three)
55
 
 
56
 
or ::
57
 
 
58
 
     self.legbone.kneebone.shinbone.toebone.shake_it(
59
 
         one, two, three)
60
 
 
61
 
For long lists, we like to add a trailing comma and put the closing
62
 
character on the following line.  This makes it easier to add new items in
63
 
future::
64
 
 
65
 
    from bzrlib.goo import (
66
 
        jam,
67
 
        jelly,
68
 
        marmalade,
69
 
        )
70
 
 
71
 
There should be spaces between function parameters, but not between the
72
 
keyword name and the value::
73
 
 
74
 
    call(1, 3, cheese=quark)
75
 
 
76
 
 
77
 
Python versions
78
 
===============
79
 
 
80
 
Bazaar supports Python from 2.4 through 2.6, and in the future we want to
81
 
support Python 3.0.  Avoid using language features added in 2.5 or 2.6, or
82
 
features deprecated in Python 3.0.  (You can check v3 compatibility using
83
 
the ``-3`` option of Python2.6.)
84
 
 
85
 
Specifically:
86
 
 
87
 
* Don't use the ``with`` statement.
88
 
 
89
 
* Don't ``from . import``.
90
 
 
91
 
* Don't use ``try/except/finally``, which is not supported in Python2.4,
92
 
  use separate nested ``try/except`` and ``try/finally`` blocks.
93
 
 
94
 
 
95
 
hasattr and getattr
96
 
===================
97
 
 
98
 
``hasattr`` should not be used because it swallows exceptions including
99
 
``KeyboardInterrupt``.  Instead, say something like ::
100
 
 
101
 
  if getattr(thing, 'name', None) is None
102
 
 
103
 
 
104
 
Module Imports
105
 
==============
106
 
 
107
 
* Imports should be done at the top-level of the file, unless there is
108
 
  a strong reason to have them lazily loaded when a particular
109
 
  function runs.  Import statements have a cost, so try to make sure
110
 
  they don't run inside hot functions.
111
 
 
112
 
* Module names should always be given fully-qualified,
113
 
  i.e. ``bzrlib.hashcache`` not just ``hashcache``.
114
 
 
115
 
 
116
 
Naming
117
 
======
118
 
 
119
 
Functions, methods or members that are relatively private are given
120
 
a leading underscore prefix.  Names without a leading underscore are
121
 
public not just across modules but to programmers using bzrlib as an
122
 
API.
123
 
 
124
 
We prefer class names to be concatenated capital words (``TestCase``)
125
 
and variables, methods and functions to be lowercase words joined by
126
 
underscores (``revision_id``, ``get_revision``).
127
 
 
128
 
For the purposes of naming some names are treated as single compound
129
 
words: "filename", "revno".
130
 
 
131
 
Consider naming classes as nouns and functions/methods as verbs.
132
 
 
133
 
Try to avoid using abbreviations in names, because there can be
134
 
inconsistency if other people use the full name.
135
 
 
136
 
 
137
 
Standard Names
138
 
==============
139
 
 
140
 
``revision_id`` not ``rev_id`` or ``revid``
141
 
 
142
 
Functions that transform one thing to another should be named ``x_to_y``
143
 
(not ``x2y`` as occurs in some old code.)
144
 
 
145
 
 
146
 
Destructors
147
 
===========
148
 
 
149
 
Python destructors (``__del__``) work differently to those of other
150
 
languages.  In particular, bear in mind that destructors may be called
151
 
immediately when the object apparently becomes unreferenced, or at some
152
 
later time, or possibly never at all.  Therefore we have restrictions on
153
 
what can be done inside them.
154
 
 
155
 
0. If you think you need to use a ``__del__`` method ask another
156
 
   developer for alternatives.  If you do need to use one, explain
157
 
   why in a comment.
158
 
 
159
 
1. Never rely on a ``__del__`` method running.  If there is code that
160
 
   must run, do it from a ``finally`` block instead.
161
 
 
162
 
2. Never ``import`` from inside a ``__del__`` method, or you may crash the
163
 
   interpreter!!
164
 
 
165
 
3. In some places we raise a warning from the destructor if the object
166
 
   has not been cleaned up or closed.  This is considered OK: the warning
167
 
   may not catch every case but it's still useful sometimes.
168
 
 
169
 
 
170
 
Cleanup methods
171
 
===============
172
 
 
173
 
Often when something has failed later code will fail too, including
174
 
cleanups invoked from ``finally`` blocks.  These secondary failures are
175
 
generally uninteresting compared to the original exception.  ``bzrlib``
176
 
has some facilities you can use to mitigate this.
177
 
 
178
 
* In ``Command`` subclasses, prefer the ``add_cleanup`` method to using
179
 
  ``try``/``finally`` blocks.  E.g. to acquire a lock and ensure it will
180
 
  always be released when the command is done::
181
 
 
182
 
    self.add_cleanup(branch.lock_read().unlock)
183
 
 
184
 
  This also avoids heavily indented code. It also makes it easier to notice
185
 
  mismatched lock/unlock pairs (and other kinds of resource
186
 
  acquire/release) because there isn't a large block of code separating
187
 
  them.
188
 
 
189
 
* Use the ``only_raises`` decorator (from ``bzrlib.decorators``) when
190
 
  defining methods that are typically called in ``finally`` blocks, such
191
 
  as ``unlock`` methods.  For example, ``@only_raises(LockNotHeld,
192
 
  LockBroken)``.  All errors that are unlikely to be a knock-on failure
193
 
  from a previous failure should be allowed.
194
 
 
195
 
* Consider using the ``OperationWithCleanups`` helper from
196
 
  ``bzrlib.cleanup`` anywhere else you have a ``finally`` block that
197
 
  might fail.
198
 
 
199
 
 
200
 
Factories
201
 
=========
202
 
 
203
 
In some places we have variables which point to callables that construct
204
 
new instances.  That is to say, they can be used a lot like class objects,
205
 
but they shouldn't be *named* like classes.  Things called ``FooBar`` should
206
 
create an instance of ``FooBar``.  A factory method that might create a
207
 
``FooBar`` or might make something else should be called ``foo_factory``.
208
 
 
209
 
 
210
 
Registries
211
 
==========
212
 
 
213
 
Several places in Bazaar use (or will use) a registry, which is a
214
 
mapping from names to objects or classes.  The registry allows for
215
 
loading in registered code only when it's needed, and keeping
216
 
associated information such as a help string or description.
217
 
 
218
 
 
219
 
InterObject and multiple dispatch
220
 
=================================
221
 
 
222
 
The ``InterObject`` provides for two-way `multiple dispatch`__: matching
223
 
up for example a source and destination repository to find the right way
224
 
to transfer data between them.
225
 
 
226
 
.. __: http://en.wikipedia.org/wiki/Multiple_dispatch
227
 
 
228
 
There is a subclass ``InterObject`` classes for each type of object that is
229
 
dispatched this way, e.g. ``InterRepository``.  Calling ``.get()`` on this
230
 
class will return an ``InterObject`` instance providing the best match for
231
 
those parameters, and this instance then has methods for operations
232
 
between the objects.
233
 
 
234
 
::
235
 
 
236
 
  inter = InterRepository.get(source_repo, target_repo)
237
 
  inter.fetch(revision_id)
238
 
 
239
 
``InterRepository`` also acts as a registry-like object for its
240
 
subclasses, and they can be added through ``.register_optimizer``.  The
241
 
right one to run is selected by asking each class, in reverse order of
242
 
registration, whether it ``.is_compatible`` with the relevant objects.
243
 
 
244
 
Lazy Imports
245
 
============
246
 
 
247
 
To make startup time faster, we use the ``bzrlib.lazy_import`` module to
248
 
delay importing modules until they are actually used. ``lazy_import`` uses
249
 
the same syntax as regular python imports. So to import a few modules in a
250
 
lazy fashion do::
251
 
 
252
 
  from bzrlib.lazy_import import lazy_import
253
 
  lazy_import(globals(), """
254
 
  import os
255
 
  import subprocess
256
 
  import sys
257
 
  import time
258
 
 
259
 
  from bzrlib import (
260
 
     errors,
261
 
     transport,
262
 
     revision as _mod_revision,
263
 
     )
264
 
  import bzrlib.transport
265
 
  import bzrlib.xml5
266
 
  """)
267
 
 
268
 
At this point, all of these exist as a ``ImportReplacer`` object, ready to
269
 
be imported once a member is accessed. Also, when importing a module into
270
 
the local namespace, which is likely to clash with variable names, it is
271
 
recommended to prefix it as ``_mod_<module>``. This makes it clearer that
272
 
the variable is a module, and these object should be hidden anyway, since
273
 
they shouldn't be imported into other namespaces.
274
 
 
275
 
While it is possible for ``lazy_import()`` to import members of a module
276
 
when using the ``from module import member`` syntax, it is recommended to
277
 
only use that syntax to load sub modules ``from module import submodule``.
278
 
This is because variables and classes can frequently be used without
279
 
needing a sub-member for example::
280
 
 
281
 
  lazy_import(globals(), """
282
 
  from module import MyClass
283
 
  """)
284
 
 
285
 
  def test(x):
286
 
      return isinstance(x, MyClass)
287
 
 
288
 
This will incorrectly fail, because ``MyClass`` is a ``ImportReplacer``
289
 
object, rather than the real class.
290
 
 
291
 
It also is incorrect to assign ``ImportReplacer`` objects to other variables.
292
 
Because the replacer only knows about the original name, it is unable to
293
 
replace other variables. The ``ImportReplacer`` class will raise an
294
 
``IllegalUseOfScopeReplacer`` exception if it can figure out that this
295
 
happened. But it requires accessing a member more than once from the new
296
 
variable, so some bugs are not detected right away.
297
 
 
298
 
 
299
 
The Null revision
300
 
=================
301
 
 
302
 
The null revision is the ancestor of all revisions.  Its revno is 0, its
303
 
revision-id is ``null:``, and its tree is the empty tree.  When referring
304
 
to the null revision, please use ``bzrlib.revision.NULL_REVISION``.  Old
305
 
code sometimes uses ``None`` for the null revision, but this practice is
306
 
being phased out.
307
 
 
308
 
 
309
 
Object string representations
310
 
=============================
311
 
 
312
 
Python prints objects using their ``__repr__`` method when they are
313
 
written to logs, exception tracebacks, or the debugger.  We want
314
 
objects to have useful representations to help in determining what went
315
 
wrong.
316
 
 
317
 
If you add a new class you should generally add a ``__repr__`` method
318
 
unless there is an adequate method in a parent class.  There should be a
319
 
test for the repr.
320
 
 
321
 
Representations should typically look like Python constructor syntax, but
322
 
they don't need to include every value in the object and they don't need
323
 
to be able to actually execute.  They're to be read by humans, not
324
 
machines.  Don't hardcode the classname in the format, so that we get the
325
 
correct value if the method is inherited by a subclass.  If you're
326
 
printing attributes of the object, including strings, you should normally
327
 
use ``%r`` syntax (to call their repr in turn).
328
 
 
329
 
Try to avoid the representation becoming more than one or two lines long.
330
 
(But balance this against including useful information, and simplicity of
331
 
implementation.)
332
 
 
333
 
Because repr methods are often called when something has already gone
334
 
wrong, they should be written somewhat more defensively than most code.
335
 
The object may be half-initialized or in some other way in an illegal
336
 
state.  The repr method shouldn't raise an exception, or it may hide the
337
 
(probably more useful) underlying exception.
338
 
 
339
 
Example::
340
 
 
341
 
    def __repr__(self):
342
 
        return '%s(%r)' % (self.__class__.__name__,
343
 
                           self._transport)
344
 
 
345
 
 
346
 
Exception handling
347
 
==================
348
 
 
349
 
A bare ``except`` statement will catch all exceptions, including ones that
350
 
really should terminate the program such as ``MemoryError`` and
351
 
``KeyboardInterrupt``.  They should rarely be used unless the exception is
352
 
later re-raised.  Even then, think about whether catching just
353
 
``Exception`` (which excludes system errors in Python2.5 and later) would
354
 
be better.
355
 
 
356
 
 
357
 
Test coverage
358
 
=============
359
 
 
360
 
All code should be exercised by the test suite.  See the `Bazaar Testing
361
 
Guide <http://doc.bazaar.canonical.com/developers/testing.html>`_ for detailed
362
 
information about writing tests.
363
 
 
364
 
 
365
 
Assertions
366
 
==========
367
 
 
368
 
Do not use the Python ``assert`` statement, either in tests or elsewhere.
369
 
A source test checks that it is not used.  It is ok to explicitly raise
370
 
AssertionError.
371
 
 
372
 
Rationale:
373
 
 
374
 
* It makes the behaviour vary depending on whether bzr is run with -O
375
 
  or not, therefore giving a chance for bugs that occur in one case or
376
 
  the other, several of which have already occurred: assertions with
377
 
  side effects, code which can't continue unless the assertion passes,
378
 
  cases where we should give the user a proper message rather than an
379
 
  assertion failure.
380
 
* It's not that much shorter than an explicit if/raise.
381
 
* It tends to lead to fuzzy thinking about whether the check is
382
 
  actually needed or not, and whether it's an internal error or not
383
 
* It tends to cause look-before-you-leap patterns.
384
 
* It's unsafe if the check is needed to protect the integrity of the
385
 
  user's data.
386
 
* It tends to give poor messages since the developer can get by with
387
 
  no explanatory text at all.
388
 
* We can't rely on people always running with -O in normal use, so we
389
 
  can't use it for tests that are actually expensive.
390
 
* Expensive checks that help developers are better turned on from the
391
 
  test suite or a -D flag.
392
 
* If used instead of ``self.assert*()`` in tests it makes them falsely
393
 
  pass with -O.
394
 
 
395
 
emacs setup
396
 
===========
397
 
 
398
 
In emacs::
399
 
 
400
 
    ;(defface my-invalid-face
401
 
    ;  '((t (:background "Red" :underline t)))
402
 
    ;  "Face used to highlight invalid constructs or other uglyties"
403
 
    ;  )
404
 
 
405
 
    (defun my-python-mode-hook ()
406
 
     ;; setup preferred indentation style.
407
 
     (setq fill-column 79)
408
 
     (setq indent-tabs-mode nil) ; no tabs, never, I will not repeat
409
 
    ;  (font-lock-add-keywords 'python-mode
410
 
    ;                         '(("^\\s *\t" . 'my-invalid-face) ; Leading tabs
411
 
    ;                            ("[ \t]+$" . 'my-invalid-face)  ; Trailing spaces
412
 
    ;                            ("^[ \t]+$" . 'my-invalid-face)); Spaces only
413
 
    ;                          )
414
 
     )
415
 
 
416
 
    (add-hook 'python-mode-hook 'my-python-mode-hook)
417
 
 
418
 
The lines beginning with ';' are comments. They can be activated
419
 
if one want to have a strong notice of some tab/space usage
420
 
violations.
421
 
 
422
 
Portability Tips
423
 
================
424
 
 
425
 
The ``bzrlib.osutils`` module has many useful helper functions, including
426
 
some more portable variants of functions in the standard library.
427
 
 
428
 
In particular, don't use ``shutil.rmtree`` unless it's acceptable for it
429
 
to fail on Windows if some files are readonly or still open elsewhere.
430
 
Use ``bzrlib.osutils.rmtree`` instead.
431
 
 
432
 
Using the ``open(..).read(..)`` or ``open(..).write(..)`` style chaining
433
 
of methods for reading or writing file content relies on garbage collection
434
 
to close the file which may keep the file open for an undefined period of
435
 
time. This may break some follow up operations like rename on Windows.
436
 
Use ``try/finally`` to explictly close the file. E.g.::
437
 
 
438
 
    f = open('foo.txt', 'w')
439
 
    try:
440
 
        f.write(s)
441
 
    finally:
442
 
        f.close()
443
 
 
444
 
 
445
 
Terminology
446
 
===========
447
 
 
448
 
Bazaar is a GNU project and uses standard GNU terminology, especially:
449
 
 
450
 
 * Use the word "Linux" to refer to the Linux kernel, not as a synechoche
451
 
   for the entire operating system.  (See `bug 528253
452
 
   <https://bugs.launchpad.net/bzr/+bug/528253>`_).
453
 
 
454
 
 * Don't say "open source" when you mean "free software".
455
 
 
456
 
..
457
 
   vim: ft=rst tw=74 ai