« back to all changes in this revision
Viewing changes to doc/developers/HACKING.txt
- browse files at revision 5252
- compare with another revision
- compare with revision 1012
- download diff
- download tarball
- view history from revision 5252
- Committer: Canonical.com Patch Queue Manager
- Date: 2010-05-24 00:49:39 UTC
- mfrom: (5225.2.15 doc)
- Revision ID: pqm@pqm.ubuntu.com-20100524004939-y06jpldj8w0w8ryh
(mbp) Better docs on contributing, code reviews, and using bzrlib.
added
removed
doc/developers/HACKING.txt
35
35
36
36
* Bug Tracker for the core product - https://bugs.launchpad.net/bzr/
37
37
38
* Blueprint Tracker for the core product - https://blueprints.launchpad.net/bzr/
39
40
38
If nothing else, perhaps you'll find inspiration in how other developers
41
39
have solved their challenges.
42
40
128
126
"Propose for merging into another branch". Select "~bzr/bzr/trunk" to hand
129
127
your changes off to the Bazaar developers for review and merging.
130
128
129
Alternatively, after pushing you can use the ``lp-propose`` command to
130
create the merge proposal.
131
131
132
Using a meaningful name for your branch will help you and the reviewer(s)
132
133
better track the submission. Use a very succint description of your submission
133
134
and prefix it with bug number if needed (lp:~mbp/bzr/484558-merge-directory
135
136
(lp:~jameinel/bzr/export-file-511987).
136
137
137
138
139
Review cover letters
140
--------------------
141
142
Please put a "cover letter" on your merge request explaining:
143
144
* the reason **why** you're making this change
145
146
* **how** this change achieves this purpose
147
148
* anything else you may have fixed in passing
149
150
* anything significant that you thought of doing, such as a more
151
extensive fix or a different approach, but didn't or couldn't do now
152
153
A good cover letter makes reviewers' lives easier because they can decide
154
from the letter whether they agree with the purpose and approach, and then
155
assess whether the patch actually does what the cover letter says.
156
Explaining any "drive-by fixes" or roads not taken may also avoid queries
157
from the reviewer. All in all this should give faster and better reviews.
158
Sometimes writing the cover letter helps the submitter realize something
159
else they need to do. The size of the cover letter should be proportional
160
to the size and complexity of the patch.
161
162
138
163
Why make a local copy of bzr.dev?
139
164
---------------------------------
140
165
270
295
<http://doc.bazaar-vcs.org/developers/overview.html>`_.
271
296
272
297
273
Coding Style Guidelines
274
#######################
275
276
hasattr and getattr
277
===================
278
279
``hasattr`` should not be used because it swallows exceptions including
280
``KeyboardInterrupt``. Instead, say something like ::
281
282
if getattr(thing, 'name', None) is None
283
284
285
Code layout
286
===========
287
288
Please write PEP-8__ compliant code.
289
290
__ http://www.python.org/peps/pep-0008.html
291
292
One often-missed requirement is that the first line of docstrings
293
should be a self-contained one-sentence summary.
294
295
We use 4 space indents for blocks, and never use tab characters. (In vim,
296
``set expandtab``.)
297
298
Trailing white space should be avoided, but is allowed.
299
You should however not make lots of unrelated white space changes.
300
301
Unix style newlines (LF) are used.
302
303
Each file must have a newline at the end of it.
304
305
Lines should be no more than 79 characters if at all possible.
306
Lines that continue a long statement may be indented in either of
307
two ways:
308
309
within the parenthesis or other character that opens the block, e.g.::
310
311
my_long_method(arg1,
312
arg2,
313
arg3)
314
315
or indented by four spaces::
316
317
my_long_method(arg1,
318
arg2,
319
arg3)
320
321
The first is considered clearer by some people; however it can be a bit
322
harder to maintain (e.g. when the method name changes), and it does not
323
work well if the relevant parenthesis is already far to the right. Avoid
324
this::
325
326
self.legbone.kneebone.shinbone.toebone.shake_it(one,
327
two,
328
three)
329
330
but rather ::
331
332
self.legbone.kneebone.shinbone.toebone.shake_it(one,
333
two,
334
three)
335
336
or ::
337
338
self.legbone.kneebone.shinbone.toebone.shake_it(
339
one, two, three)
340
341
For long lists, we like to add a trailing comma and put the closing
342
character on the following line. This makes it easier to add new items in
343
future::
344
345
from bzrlib.goo import (
346
jam,
347
jelly,
348
marmalade,
349
)
350
351
There should be spaces between function parameters, but not between the
352
keyword name and the value::
353
354
call(1, 3, cheese=quark)
355
356
In emacs::
357
358
;(defface my-invalid-face
359
; '((t (:background "Red" :underline t)))
360
; "Face used to highlight invalid constructs or other uglyties"
361
; )
362
363
(defun my-python-mode-hook ()
364
;; setup preferred indentation style.
365
(setq fill-column 79)
366
(setq indent-tabs-mode nil) ; no tabs, never, I will not repeat
367
; (font-lock-add-keywords 'python-mode
368
; '(("^\\s *\t" . 'my-invalid-face) ; Leading tabs
369
; ("[ \t]+$" . 'my-invalid-face) ; Trailing spaces
370
; ("^[ \t]+$" . 'my-invalid-face)); Spaces only
371
; )
372
)
373
374
(add-hook 'python-mode-hook 'my-python-mode-hook)
375
376
The lines beginning with ';' are comments. They can be activated
377
if one want to have a strong notice of some tab/space usage
378
violations.
379
380
381
Module Imports
382
==============
383
384
* Imports should be done at the top-level of the file, unless there is
385
a strong reason to have them lazily loaded when a particular
386
function runs. Import statements have a cost, so try to make sure
387
they don't run inside hot functions.
388
389
* Module names should always be given fully-qualified,
390
i.e. ``bzrlib.hashcache`` not just ``hashcache``.
391
392
393
Naming
394
======
395
396
Functions, methods or members that are relatively private are given
397
a leading underscore prefix. Names without a leading underscore are
398
public not just across modules but to programmers using bzrlib as an
399
API.
400
401
We prefer class names to be concatenated capital words (``TestCase``)
402
and variables, methods and functions to be lowercase words joined by
403
underscores (``revision_id``, ``get_revision``).
404
405
For the purposes of naming some names are treated as single compound
406
words: "filename", "revno".
407
408
Consider naming classes as nouns and functions/methods as verbs.
409
410
Try to avoid using abbreviations in names, because there can be
411
inconsistency if other people use the full name.
412
413
414
Standard Names
415
==============
416
417
``revision_id`` not ``rev_id`` or ``revid``
418
419
Functions that transform one thing to another should be named ``x_to_y``
420
(not ``x2y`` as occurs in some old code.)
421
422
423
Destructors
424
===========
425
426
Python destructors (``__del__``) work differently to those of other
427
languages. In particular, bear in mind that destructors may be called
428
immediately when the object apparently becomes unreferenced, or at some
429
later time, or possibly never at all. Therefore we have restrictions on
430
what can be done inside them.
431
432
0. If you think you need to use a ``__del__`` method ask another
433
developer for alternatives. If you do need to use one, explain
434
why in a comment.
435
436
1. Never rely on a ``__del__`` method running. If there is code that
437
must run, do it from a ``finally`` block instead.
438
439
2. Never ``import`` from inside a ``__del__`` method, or you may crash the
440
interpreter!!
441
442
3. In some places we raise a warning from the destructor if the object
443
has not been cleaned up or closed. This is considered OK: the warning
444
may not catch every case but it's still useful sometimes.
445
446
447
Cleanup methods
448
===============
449
450
Often when something has failed later code, including cleanups invoked
451
from ``finally`` blocks, will fail too. These secondary failures are
452
generally uninteresting compared to the original exception. So use the
453
``only_raises`` decorator (from ``bzrlib.decorators``) for methods that
454
are typically called in ``finally`` blocks, such as ``unlock`` methods.
455
For example, ``@only_raises(LockNotHeld, LockBroken)``. All errors that
456
are unlikely to be a knock-on failure from a previous failure should be
457
allowed.
458
459
460
Factories
461
=========
462
463
In some places we have variables which point to callables that construct
464
new instances. That is to say, they can be used a lot like class objects,
465
but they shouldn't be *named* like classes::
466
467
> I think that things named FooBar should create instances of FooBar when
468
> called. Its plain confusing for them to do otherwise. When we have
469
> something that is going to be used as a class - that is, checked for via
470
> isinstance or other such idioms, them I would call it foo_class, so that
471
> it is clear that a callable is not sufficient. If it is only used as a
472
> factory, then yes, foo_factory is what I would use.
473
474
475
Registries
476
==========
477
478
Several places in Bazaar use (or will use) a registry, which is a
479
mapping from names to objects or classes. The registry allows for
480
loading in registered code only when it's needed, and keeping
481
associated information such as a help string or description.
482
483
484
InterObject and multiple dispatch
485
=================================
486
487
The ``InterObject`` provides for two-way `multiple dispatch`__: matching
488
up for example a source and destination repository to find the right way
489
to transfer data between them.
490
491
.. __: http://en.wikipedia.org/wiki/Multiple_dispatch
492
493
There is a subclass ``InterObject`` classes for each type of object that is
494
dispatched this way, e.g. ``InterRepository``. Calling ``.get()`` on this
495
class will return an ``InterObject`` instance providing the best match for
496
those parameters, and this instance then has methods for operations
497
between the objects.
498
499
::
500
501
inter = InterRepository.get(source_repo, target_repo)
502
inter.fetch(revision_id)
503
504
``InterRepository`` also acts as a registry-like object for its
505
subclasses, and they can be added through ``.register_optimizer``. The
506
right one to run is selected by asking each class, in reverse order of
507
registration, whether it ``.is_compatible`` with the relevant objects.
508
509
Lazy Imports
510
============
511
512
To make startup time faster, we use the ``bzrlib.lazy_import`` module to
513
delay importing modules until they are actually used. ``lazy_import`` uses
514
the same syntax as regular python imports. So to import a few modules in a
515
lazy fashion do::
516
517
from bzrlib.lazy_import import lazy_import
518
lazy_import(globals(), """
519
import os
520
import subprocess
521
import sys
522
import time
523
524
from bzrlib import (
525
errors,
526
transport,
527
revision as _mod_revision,
528
)
529
import bzrlib.transport
530
import bzrlib.xml5
531
""")
532
533
At this point, all of these exist as a ``ImportReplacer`` object, ready to
534
be imported once a member is accessed. Also, when importing a module into
535
the local namespace, which is likely to clash with variable names, it is
536
recommended to prefix it as ``_mod_<module>``. This makes it clearer that
537
the variable is a module, and these object should be hidden anyway, since
538
they shouldn't be imported into other namespaces.
539
540
While it is possible for ``lazy_import()`` to import members of a module
541
when using the ``from module import member`` syntax, it is recommended to
542
only use that syntax to load sub modules ``from module import submodule``.
543
This is because variables and classes can frequently be used without
544
needing a sub-member for example::
545
546
lazy_import(globals(), """
547
from module import MyClass
548
""")
549
550
def test(x):
551
return isinstance(x, MyClass)
552
553
This will incorrectly fail, because ``MyClass`` is a ``ImportReplacer``
554
object, rather than the real class.
555
556
It also is incorrect to assign ``ImportReplacer`` objects to other variables.
557
Because the replacer only knows about the original name, it is unable to
558
replace other variables. The ``ImportReplacer`` class will raise an
559
``IllegalUseOfScopeReplacer`` exception if it can figure out that this
560
happened. But it requires accessing a member more than once from the new
561
variable, so some bugs are not detected right away.
562
563
564
The Null revision
565
=================
566
567
The null revision is the ancestor of all revisions. Its revno is 0, its
568
revision-id is ``null:``, and its tree is the empty tree. When referring
569
to the null revision, please use ``bzrlib.revision.NULL_REVISION``. Old
570
code sometimes uses ``None`` for the null revision, but this practice is
571
being phased out.
572
573
574
Object string representations
575
=============================
576
577
Python prints objects using their ``__repr__`` method when they are
578
written to logs, exception tracebacks, or the debugger. We want
579
objects to have useful representations to help in determining what went
580
wrong.
581
582
If you add a new class you should generally add a ``__repr__`` method
583
unless there is an adequate method in a parent class. There should be a
584
test for the repr.
585
586
Representations should typically look like Python constructor syntax, but
587
they don't need to include every value in the object and they don't need
588
to be able to actually execute. They're to be read by humans, not
589
machines. Don't hardcode the classname in the format, so that we get the
590
correct value if the method is inherited by a subclass. If you're
591
printing attributes of the object, including strings, you should normally
592
use ``%r`` syntax (to call their repr in turn).
593
594
Try to avoid the representation becoming more than one or two lines long.
595
(But balance this against including useful information, and simplicity of
596
implementation.)
597
598
Because repr methods are often called when something has already gone
599
wrong, they should be written somewhat more defensively than most code.
600
The object may be half-initialized or in some other way in an illegal
601
state. The repr method shouldn't raise an exception, or it may hide the
602
(probably more useful) underlying exception.
603
604
Example::
605
606
def __repr__(self):
607
return '%s(%r)' % (self.__class__.__name__,
608
self._transport)
609
610
611
Exception handling
612
==================
613
614
A bare ``except`` statement will catch all exceptions, including ones that
615
really should terminate the program such as ``MemoryError`` and
616
``KeyboardInterrupt``. They should rarely be used unless the exception is
617
later re-raised. Even then, think about whether catching just
618
``Exception`` (which excludes system errors in Python2.5 and later) would
619
be better.
620
621
622
Test coverage
623
=============
624
625
All code should be exercised by the test suite. See the `Bazaar Testing
626
Guide <http://doc.bazaar-vcs.org/developers/testing.html>`_ for detailed
627
information about writing tests.
628
629
630
298
Core Topics
631
299
###########
632
300
891
559
final fullstop. If long, they may contain newlines to break the text.
892
560
893
561
894
Assertions
895
==========
896
897
Do not use the Python ``assert`` statement, either in tests or elsewhere.
898
A source test checks that it is not used. It is ok to explicitly raise
899
AssertionError.
900
901
Rationale:
902
903
* It makes the behaviour vary depending on whether bzr is run with -O
904
or not, therefore giving a chance for bugs that occur in one case or
905
the other, several of which have already occurred: assertions with
906
side effects, code which can't continue unless the assertion passes,
907
cases where we should give the user a proper message rather than an
908
assertion failure.
909
* It's not that much shorter than an explicit if/raise.
910
* It tends to lead to fuzzy thinking about whether the check is
911
actually needed or not, and whether it's an internal error or not
912
* It tends to cause look-before-you-leap patterns.
913
* It's unsafe if the check is needed to protect the integrity of the
914
user's data.
915
* It tends to give poor messages since the developer can get by with
916
no explanatory text at all.
917
* We can't rely on people always running with -O in normal use, so we
918
can't use it for tests that are actually expensive.
919
* Expensive checks that help developers are better turned on from the
920
test suite or a -D flag.
921
* If used instead of ``self.assert*()`` in tests it makes them falsely pass with -O.
922
923
562
924
563
Documenting Changes
925
564
===================
1143
782
valid characters are generated where possible.
1144
783
1145
784
1146
Portability Tips
1147
================
1148
1149
The ``bzrlib.osutils`` module has many useful helper functions, including
1150
some more portable variants of functions in the standard library.
1151
1152
In particular, don't use ``shutil.rmtree`` unless it's acceptable for it
1153
to fail on Windows if some files are readonly or still open elsewhere.
1154
Use ``bzrlib.osutils.rmtree`` instead.
1155
1156
1157
785
C Extension Modules
1158
786
===================
1159
787
1411
1039
results.
1412
1040
1413
1041
1414
Reviewing Blueprints
1415
====================
1416
1417
Blueprint Tracking Using Launchpad
1418
----------------------------------
1419
1420
New features typically require a fair amount of discussion, design and
1421
debate. For Bazaar, that information is often captured in a so-called
1422
"blueprint" on our Wiki. Overall tracking of blueprints and their status
1423
is done using Launchpad's relevant tracker,
1424
https://blueprints.launchpad.net/bzr/. Once a blueprint for ready for
1425
review, please announce it on the mailing list.
1426
1427
Alternatively, send an email beginning with [RFC] with the proposal to the
1428
list. In some cases, you may wish to attach proposed code or a proposed
1429
developer document if that best communicates the idea. Debate can then
1430
proceed using the normal merge review processes.
1431
1432
1433
Recording Blueprint Review Feedback
1434
-----------------------------------
1435
1436
Unlike its Bug Tracker, Launchpad's Blueprint Tracker doesn't currently
1437
(Jun 2007) support a chronological list of comment responses. Review
1438
feedback can either be recorded on the Wiki hosting the blueprints or by
1439
using Launchpad's whiteboard feature.
1440
1441
1442
1042
Planning Releases
1443
1043
=================
1444
1044
1445
1045
1446
Using Releases and Milestones in Launchpad
1447
------------------------------------------
1448
1449
TODO ... (Exact policies still under discussion)
1450
1451
1452
1046
Bug Triage
1453
1047
----------
1454
1048
Loggerhead is a web-based interface for Breezy
Version: 2.0.1