~bzr-pqm/bzr/bzr.dev

5225.2.9 by Martin Pool
Split out code style guide from HACKING
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
5225.2.10 by Martin Pool
More code style guidelines cleanups
76
5225.2.11 by Martin Pool
Style guide point about python versions
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
5274.3.1 by Andrew Bennetts
Expand 'Cleanup methods' section of coding style guide, and also correct some excessively indented bullets.
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.
5225.2.11 by Martin Pool
Style guide point about python versions
93
94
5225.2.10 by Martin Pool
More code style guidelines cleanups
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
5225.2.9 by Martin Pool
Split out code style guide from HACKING
102
103
5418.2.1 by Martin Pool
Text about kwargs from spiv
104
kwargs
105
======
106
107
``**kwargs`` in the prototype of a function should be used sparingly.
108
It can be good on higher-order functions that decorate other functions,
5418.2.5 by Martin Pool
Cleanup style of developer advice about kwargs
109
such as ``addCleanup`` or ``assertRaises``, or on functions that take only
110
(or almost only) kwargs, where any kwargs can be passed.  
5418.2.1 by Martin Pool
Text about kwargs from spiv
111
5418.2.5 by Martin Pool
Cleanup style of developer advice about kwargs
112
Otherwise, be careful: if the parameters to a function are a bit complex
113
and might vary over time (e.g.  the ``commit`` API) then we prefer to pass an
114
object rather than a bag of positional and/or keyword args.  If you have
115
an arbitrary set of keys and values that are different with each use (e.g.
116
string interpolation inputs) then again that should not be mixed in with
117
the regular positional/keyword args, it seems like a different category of
118
thing.
5418.2.1 by Martin Pool
Text about kwargs from spiv
119
120
5418.2.3 by Martin Pool
Code guideline about imitating standard objects
121
Imitating standard objects
122
==========================
123
124
Don't provide methods that imitate built-in classes (eg ``__in__``,
125
``__call__``, ``__int__``, ``__getitem__``) unless the class you're
126
implementing really does act like the builtin class, in semantics and
127
performance.
128
129
For example, old code lets you say ``file_id in inv`` but we no longer
130
consider this good style.  Instead, say more explicitly
131
``inv.has_id(file_id)``.
132
133
``__repr__``, ``__cmp__``, ``__str__`` are usually fine.
134
135
5225.2.9 by Martin Pool
Split out code style guide from HACKING
136
Module Imports
137
==============
138
139
* Imports should be done at the top-level of the file, unless there is
140
  a strong reason to have them lazily loaded when a particular
141
  function runs.  Import statements have a cost, so try to make sure
142
  they don't run inside hot functions.
143
144
* Module names should always be given fully-qualified,
145
  i.e. ``bzrlib.hashcache`` not just ``hashcache``.
146
147
148
Naming
149
======
150
151
Functions, methods or members that are relatively private are given
152
a leading underscore prefix.  Names without a leading underscore are
153
public not just across modules but to programmers using bzrlib as an
154
API.
155
156
We prefer class names to be concatenated capital words (``TestCase``)
157
and variables, methods and functions to be lowercase words joined by
158
underscores (``revision_id``, ``get_revision``).
159
160
For the purposes of naming some names are treated as single compound
161
words: "filename", "revno".
162
163
Consider naming classes as nouns and functions/methods as verbs.
164
165
Try to avoid using abbreviations in names, because there can be
166
inconsistency if other people use the full name.
167
168
169
Standard Names
170
==============
171
172
``revision_id`` not ``rev_id`` or ``revid``
173
174
Functions that transform one thing to another should be named ``x_to_y``
175
(not ``x2y`` as occurs in some old code.)
176
177
178
Destructors
179
===========
180
181
Python destructors (``__del__``) work differently to those of other
182
languages.  In particular, bear in mind that destructors may be called
183
immediately when the object apparently becomes unreferenced, or at some
184
later time, or possibly never at all.  Therefore we have restrictions on
185
what can be done inside them.
186
5274.3.1 by Andrew Bennetts
Expand 'Cleanup methods' section of coding style guide, and also correct some excessively indented bullets.
187
0. If you think you need to use a ``__del__`` method ask another
188
   developer for alternatives.  If you do need to use one, explain
189
   why in a comment.
190
191
1. Never rely on a ``__del__`` method running.  If there is code that
192
   must run, do it from a ``finally`` block instead.
193
194
2. Never ``import`` from inside a ``__del__`` method, or you may crash the
195
   interpreter!!
196
197
3. In some places we raise a warning from the destructor if the object
198
   has not been cleaned up or closed.  This is considered OK: the warning
199
   may not catch every case but it's still useful sometimes.
5225.2.9 by Martin Pool
Split out code style guide from HACKING
200
201
202
Cleanup methods
203
===============
204
5274.3.1 by Andrew Bennetts
Expand 'Cleanup methods' section of coding style guide, and also correct some excessively indented bullets.
205
Often when something has failed later code will fail too, including
206
cleanups invoked from ``finally`` blocks.  These secondary failures are
207
generally uninteresting compared to the original exception.  ``bzrlib``
208
has some facilities you can use to mitigate this.
209
210
* In ``Command`` subclasses, prefer the ``add_cleanup`` method to using
211
  ``try``/``finally`` blocks.  E.g. to acquire a lock and ensure it will
212
  always be released when the command is done::
213
214
    self.add_cleanup(branch.lock_read().unlock)
215
216
  This also avoids heavily indented code. It also makes it easier to notice
217
  mismatched lock/unlock pairs (and other kinds of resource
218
  acquire/release) because there isn't a large block of code separating
219
  them.
220
221
* Use the ``only_raises`` decorator (from ``bzrlib.decorators``) when
222
  defining methods that are typically called in ``finally`` blocks, such
223
  as ``unlock`` methods.  For example, ``@only_raises(LockNotHeld,
224
  LockBroken)``.  All errors that are unlikely to be a knock-on failure
225
  from a previous failure should be allowed.
226
227
* Consider using the ``OperationWithCleanups`` helper from
228
  ``bzrlib.cleanup`` anywhere else you have a ``finally`` block that
229
  might fail.
5225.2.9 by Martin Pool
Split out code style guide from HACKING
230
231
232
Factories
233
=========
234
235
In some places we have variables which point to callables that construct
236
new instances.  That is to say, they can be used a lot like class objects,
5225.2.10 by Martin Pool
More code style guidelines cleanups
237
but they shouldn't be *named* like classes.  Things called ``FooBar`` should
238
create an instance of ``FooBar``.  A factory method that might create a
239
``FooBar`` or might make something else should be called ``foo_factory``.
5225.2.9 by Martin Pool
Split out code style guide from HACKING
240
241
242
Registries
243
==========
244
245
Several places in Bazaar use (or will use) a registry, which is a
246
mapping from names to objects or classes.  The registry allows for
247
loading in registered code only when it's needed, and keeping
248
associated information such as a help string or description.
249
250
251
InterObject and multiple dispatch
252
=================================
253
254
The ``InterObject`` provides for two-way `multiple dispatch`__: matching
255
up for example a source and destination repository to find the right way
256
to transfer data between them.
257
258
.. __: http://en.wikipedia.org/wiki/Multiple_dispatch
259
260
There is a subclass ``InterObject`` classes for each type of object that is
261
dispatched this way, e.g. ``InterRepository``.  Calling ``.get()`` on this
262
class will return an ``InterObject`` instance providing the best match for
263
those parameters, and this instance then has methods for operations
264
between the objects.
265
266
::
267
268
  inter = InterRepository.get(source_repo, target_repo)
269
  inter.fetch(revision_id)
270
271
``InterRepository`` also acts as a registry-like object for its
272
subclasses, and they can be added through ``.register_optimizer``.  The
273
right one to run is selected by asking each class, in reverse order of
274
registration, whether it ``.is_compatible`` with the relevant objects.
275
276
Lazy Imports
277
============
278
279
To make startup time faster, we use the ``bzrlib.lazy_import`` module to
280
delay importing modules until they are actually used. ``lazy_import`` uses
281
the same syntax as regular python imports. So to import a few modules in a
282
lazy fashion do::
283
284
  from bzrlib.lazy_import import lazy_import
285
  lazy_import(globals(), """
286
  import os
287
  import subprocess
288
  import sys
289
  import time
290
291
  from bzrlib import (
292
     errors,
293
     transport,
294
     revision as _mod_revision,
295
     )
296
  import bzrlib.transport
297
  import bzrlib.xml5
298
  """)
299
300
At this point, all of these exist as a ``ImportReplacer`` object, ready to
301
be imported once a member is accessed. Also, when importing a module into
302
the local namespace, which is likely to clash with variable names, it is
303
recommended to prefix it as ``_mod_<module>``. This makes it clearer that
304
the variable is a module, and these object should be hidden anyway, since
305
they shouldn't be imported into other namespaces.
306
307
While it is possible for ``lazy_import()`` to import members of a module
308
when using the ``from module import member`` syntax, it is recommended to
309
only use that syntax to load sub modules ``from module import submodule``.
310
This is because variables and classes can frequently be used without
311
needing a sub-member for example::
312
313
  lazy_import(globals(), """
314
  from module import MyClass
315
  """)
316
317
  def test(x):
318
      return isinstance(x, MyClass)
319
320
This will incorrectly fail, because ``MyClass`` is a ``ImportReplacer``
321
object, rather than the real class.
322
323
It also is incorrect to assign ``ImportReplacer`` objects to other variables.
324
Because the replacer only knows about the original name, it is unable to
325
replace other variables. The ``ImportReplacer`` class will raise an
326
``IllegalUseOfScopeReplacer`` exception if it can figure out that this
327
happened. But it requires accessing a member more than once from the new
328
variable, so some bugs are not detected right away.
329
330
331
The Null revision
332
=================
333
334
The null revision is the ancestor of all revisions.  Its revno is 0, its
335
revision-id is ``null:``, and its tree is the empty tree.  When referring
336
to the null revision, please use ``bzrlib.revision.NULL_REVISION``.  Old
337
code sometimes uses ``None`` for the null revision, but this practice is
338
being phased out.
339
340
341
Object string representations
342
=============================
343
344
Python prints objects using their ``__repr__`` method when they are
345
written to logs, exception tracebacks, or the debugger.  We want
346
objects to have useful representations to help in determining what went
347
wrong.
348
349
If you add a new class you should generally add a ``__repr__`` method
350
unless there is an adequate method in a parent class.  There should be a
351
test for the repr.
352
353
Representations should typically look like Python constructor syntax, but
354
they don't need to include every value in the object and they don't need
355
to be able to actually execute.  They're to be read by humans, not
356
machines.  Don't hardcode the classname in the format, so that we get the
357
correct value if the method is inherited by a subclass.  If you're
358
printing attributes of the object, including strings, you should normally
359
use ``%r`` syntax (to call their repr in turn).
360
361
Try to avoid the representation becoming more than one or two lines long.
362
(But balance this against including useful information, and simplicity of
363
implementation.)
364
365
Because repr methods are often called when something has already gone
366
wrong, they should be written somewhat more defensively than most code.
367
The object may be half-initialized or in some other way in an illegal
368
state.  The repr method shouldn't raise an exception, or it may hide the
369
(probably more useful) underlying exception.
370
371
Example::
372
373
    def __repr__(self):
374
        return '%s(%r)' % (self.__class__.__name__,
375
                           self._transport)
376
377
378
Exception handling
379
==================
380
381
A bare ``except`` statement will catch all exceptions, including ones that
382
really should terminate the program such as ``MemoryError`` and
383
``KeyboardInterrupt``.  They should rarely be used unless the exception is
384
later re-raised.  Even then, think about whether catching just
385
``Exception`` (which excludes system errors in Python2.5 and later) would
386
be better.
387
388
389
Test coverage
390
=============
391
392
All code should be exercised by the test suite.  See the `Bazaar Testing
5261.2.1 by Parth Malwankar
added 'Portability Tip' on explicitly closing file to code-style.
393
Guide <http://doc.bazaar.canonical.com/developers/testing.html>`_ for detailed
5225.2.9 by Martin Pool
Split out code style guide from HACKING
394
information about writing tests.
395
5225.2.10 by Martin Pool
More code style guidelines cleanups
396
397
Assertions
398
==========
399
400
Do not use the Python ``assert`` statement, either in tests or elsewhere.
401
A source test checks that it is not used.  It is ok to explicitly raise
402
AssertionError.
403
404
Rationale:
405
5274.3.1 by Andrew Bennetts
Expand 'Cleanup methods' section of coding style guide, and also correct some excessively indented bullets.
406
* It makes the behaviour vary depending on whether bzr is run with -O
407
  or not, therefore giving a chance for bugs that occur in one case or
408
  the other, several of which have already occurred: assertions with
409
  side effects, code which can't continue unless the assertion passes,
410
  cases where we should give the user a proper message rather than an
411
  assertion failure.
412
* It's not that much shorter than an explicit if/raise.
413
* It tends to lead to fuzzy thinking about whether the check is
414
  actually needed or not, and whether it's an internal error or not
415
* It tends to cause look-before-you-leap patterns.
416
* It's unsafe if the check is needed to protect the integrity of the
417
  user's data.
418
* It tends to give poor messages since the developer can get by with
419
  no explanatory text at all.
420
* We can't rely on people always running with -O in normal use, so we
421
  can't use it for tests that are actually expensive.
422
* Expensive checks that help developers are better turned on from the
423
  test suite or a -D flag.
424
* If used instead of ``self.assert*()`` in tests it makes them falsely
425
  pass with -O.
5225.2.10 by Martin Pool
More code style guidelines cleanups
426
427
emacs setup
428
===========
429
430
In emacs::
431
432
    ;(defface my-invalid-face
433
    ;  '((t (:background "Red" :underline t)))
434
    ;  "Face used to highlight invalid constructs or other uglyties"
435
    ;  )
436
437
    (defun my-python-mode-hook ()
438
     ;; setup preferred indentation style.
439
     (setq fill-column 79)
440
     (setq indent-tabs-mode nil) ; no tabs, never, I will not repeat
441
    ;  (font-lock-add-keywords 'python-mode
442
    ;                         '(("^\\s *\t" . 'my-invalid-face) ; Leading tabs
443
    ;                            ("[ \t]+$" . 'my-invalid-face)  ; Trailing spaces
444
    ;                            ("^[ \t]+$" . 'my-invalid-face)); Spaces only
445
    ;                          )
446
     )
447
448
    (add-hook 'python-mode-hook 'my-python-mode-hook)
449
450
The lines beginning with ';' are comments. They can be activated
451
if one want to have a strong notice of some tab/space usage
452
violations.
5225.2.13 by Martin Pool
More reorganization of the developer documentation
453
454
Portability Tips
455
================
456
457
The ``bzrlib.osutils`` module has many useful helper functions, including
458
some more portable variants of functions in the standard library.
459
460
In particular, don't use ``shutil.rmtree`` unless it's acceptable for it
461
to fail on Windows if some files are readonly or still open elsewhere.
462
Use ``bzrlib.osutils.rmtree`` instead.
463
5261.2.1 by Parth Malwankar
added 'Portability Tip' on explicitly closing file to code-style.
464
Using the ``open(..).read(..)`` or ``open(..).write(..)`` style chaining
465
of methods for reading or writing file content relies on garbage collection
466
to close the file which may keep the file open for an undefined period of
467
time. This may break some follow up operations like rename on Windows.
468
Use ``try/finally`` to explictly close the file. E.g.::
5225.2.13 by Martin Pool
More reorganization of the developer documentation
469
5261.2.1 by Parth Malwankar
added 'Portability Tip' on explicitly closing file to code-style.
470
    f = open('foo.txt', 'w')
471
    try:
472
        f.write(s)
473
    finally:
474
        f.close()
5225.2.13 by Martin Pool
More reorganization of the developer documentation
475
5278.1.1 by Martin Pool
Call out a couple of GNU policy points about naming (gnu/linux etc)
476
477
Terminology
478
===========
479
480
Bazaar is a GNU project and uses standard GNU terminology, especially:
481
482
 * Use the word "Linux" to refer to the Linux kernel, not as a synechoche
483
   for the entire operating system.  (See `bug 528253
484
   <https://bugs.launchpad.net/bzr/+bug/528253>`_).
485
486
 * Don't say "open source" when you mean "free software".
487
5225.2.13 by Martin Pool
More reorganization of the developer documentation
488
..
489
   vim: ft=rst tw=74 ai