/brz/remove-bazaar

To get this branch, use:
bzr branch http://gegoxaren.bato24.eu/bzr/brz/remove-bazaar

« back to all changes in this revision

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

  • Committer: Jelmer Vernooij
  • Date: 2020-04-05 19:11:34 UTC
  • mto: (7490.7.16 work)
  • mto: This revision was merged to the branch mainline in revision 7501.
  • Revision ID: jelmer@jelmer.uk-20200405191134-0aebh8ikiwygxma5
Populate the .gitignore file.

Show diffs side-by-side

added added

removed removed

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