1
Reviewing proposed changes to Bazaar
2
####################################
4
All non-trivial code changes coming in to Bazaar are reviewed by someone else.
6
Anyone is welcome to review any patch. You don't need to have a full
7
understanding of the codebase to find problems in the code, the documentation,
8
or the concept of the patch.
10
Normally changes by core contributors are reviewed by one other core
11
developer, and changes from other people are reviewed by two core
12
developers. Use intelligent discretion about whether the patch is trivial.
14
No one likes their merge requests sitting in a queue going nowhere: this
15
is pure waste. We prioritize reviewing existing proposals.
16
Canonical dedicates some staff time to providing prompt helpful reviews.
17
(See <http://wiki.bazaar.canonical.com/PatchPilot/>.)
19
From late 2009 on, we do all our code reviews through Launchpad's
20
merge proposal interface.
23
Reviewing proposed changes
24
==========================
26
There are three main requirements for code to get in:
28
* Doesn't reduce test coverage: if it adds new methods or commands,
29
there should be tests for them. There is a good test framework
30
and plenty of examples to crib from, but if you are having trouble
31
working out how to test something feel free to post a draft patch
34
* Doesn't reduce design clarity, such as by entangling objects
35
we're trying to separate. This is mostly something the more
36
experienced reviewers need to help check.
38
* Improves bugs, features, speed, or code simplicity.
40
Code that goes in should not degrade any of these aspects. Patches are
41
welcome that only cleanup the code without changing the external
42
behaviour. The core developers take care to keep the code quality high
43
and understandable while recognising that perfect is sometimes the enemy
46
It is easy for reviews to make people notice other things which should be
47
fixed but those things should not hold up the original fix being accepted.
48
New things can easily be recorded in the bug tracker instead.
50
It's normally much easier to review several smaller patches than one large
51
one. You might want to use ``bzr-loom`` to maintain threads of related
52
work, or submit a preparatory patch that will make your "real" change
56
Checklist for reviewers
57
=======================
59
* Do you understand what the code's doing and why?
61
* Will it perform reasonably for large inputs, both in memory size and
62
run time? Are there some scenarios where performance should be
65
* Is it tested, and are the tests at the right level? Are there both
66
blackbox (command-line level) and API-oriented tests?
68
* If this change will be visible to end users or API users, is it
69
appropriately documented in NEWS and/or in whats-new ?
71
* Does it meet the `coding standards <code-style.html>`_?
73
* If it changes the user-visible behaviour, does it update the help
74
strings and user documentation?
76
* If it adds a new major concept or standard practice, does it update the
77
developer documentation?
79
* (your ideas here...)
85
Anyone can propose or comment on a merge proposal just by creating a
88
From <https://code.launchpad.net/bzr/+activereviews> you can see all
89
currently active reviews, and choose one to comment on. This page also
90
shows proposals that are now approved and should be merged by someone with
93
<https://help.launchpad.net/Code/Review> explains the various merge proposal
94
states. Note that we don't use state *Approved* until the patch is completely
98
Landing approved changes
99
========================
101
Once a merge proposal is approved and finished, it's sent to PQM (the patch
102
queue manager) which will automatically test and integrate it. The recommended
103
way to start this off is by running the ``feed-pqm`` script from
104
<https://launchpad.net/hydrazine/>.
added
removed
doc/developers/code-review.txt
