« back to all changes in this revision
Viewing changes to doc/developers/code-review.txt
- browse files at revision 5227
- compare with another revision
- compare with revision 5227
- download diff
- download tarball
- view history from revision 5227
- Committer: Canonical.com Patch Queue Manager
- Date: 2010-05-12 13:57:49 UTC
- mfrom: (5225.2.3 doc)
- Revision ID: pqm@pqm.ubuntu.com-20100512135749-8b0et7ntvh8dgm2t
(mbp) (mbp) mention babune in testing guide;
split out review guide (Martin Pool)
split out review guide (Martin Pool)
added
removed
doc/developers/code-review.txt
1
The Code Review Process
2
#######################
3
4
All code changes coming in to Bazaar are reviewed by someone else.
5
Normally changes by core contributors are reviewed by one other core
6
developer, and changes from other people are reviewed by two core
7
developers. Use intelligent discretion if the patch is trivial.
8
9
Good reviews do take time. They also regularly require a solid
10
understanding of the overall code base. In practice, this means a small
11
number of people often have a large review burden - with knowledge comes
12
responsibility. No one likes their merge requests sitting in a queue going
13
nowhere, so reviewing sooner rather than later is strongly encouraged.
14
15
16
17
Review cover letters
18
====================
19
20
Please put a "cover letter" on your merge request explaining:
21
22
* the reason **why** you're making this change
23
24
* **how** this change achieves this purpose
25
26
* anything else you may have fixed in passing
27
28
* anything significant that you thought of doing, such as a more
29
extensive fix or a different approach, but didn't or couldn't do now
30
31
A good cover letter makes reviewers' lives easier because they can decide
32
from the letter whether they agree with the purpose and approach, and then
33
assess whether the patch actually does what the cover letter says.
34
Explaining any "drive-by fixes" or roads not taken may also avoid queries
35
from the reviewer. All in all this should give faster and better reviews.
36
Sometimes writing the cover letter helps the submitter realize something
37
else they need to do. The size of the cover letter should be proportional
38
to the size and complexity of the patch.
39
40
41
Reviewing proposed changes
42
==========================
43
44
Anyone is welcome to review code, and reply to the thread with their
45
opinion or comments.
46
47
The simplest way to review a proposed change is to just read the patch on
48
the list or in Bundle Buggy. For more complex changes it may be useful
49
to make a new working tree or branch from trunk, and merge the proposed
50
change into it, so you can experiment with the code or look at a wider
51
context.
52
53
There are three main requirements for code to get in:
54
55
* Doesn't reduce test coverage: if it adds new methods or commands,
56
there should be tests for them. There is a good test framework
57
and plenty of examples to crib from, but if you are having trouble
58
working out how to test something feel free to post a draft patch
59
and ask for help.
60
61
* Doesn't reduce design clarity, such as by entangling objects
62
we're trying to separate. This is mostly something the more
63
experienced reviewers need to help check.
64
65
* Improves bugs, features, speed, or code simplicity.
66
67
Code that goes in should not degrade any of these aspects. Patches are
68
welcome that only cleanup the code without changing the external
69
behaviour. The core developers take care to keep the code quality high
70
and understandable while recognising that perfect is sometimes the enemy
71
of good.
72
73
It is easy for reviews to make people notice other things which should be
74
fixed but those things should not hold up the original fix being accepted.
75
New things can easily be recorded in the Bug Tracker instead.
76
77
It's normally much easier to review several smaller patches than one large
78
one. You might want to use ``bzr-loom`` to maintain threads of related
79
work, or submit a preparatory patch that will make your "real" change
80
easier.
81
82
83
Checklist for reviewers
84
=======================
85
86
* Do you understand what the code's doing and why?
87
88
* Will it perform reasonably for large inputs, both in memory size and
89
run time? Are there some scenarios where performance should be
90
measured?
91
92
* Is it tested, and are the tests at the right level? Are there both
93
blackbox (command-line level) and API-oriented tests?
94
95
* If this change will be visible to end users or API users, is it
96
appropriately documented in NEWS?
97
98
* Does it meet the coding standards below?
99
100
* If it changes the user-visible behaviour, does it update the help
101
strings and user documentation?
102
103
* If it adds a new major concept or standard practice, does it update the
104
developer documentation?
105
106
* (your ideas here...)
107
108
109
Reviews on Launchpad
110
====================
111
112
From May 2009 on, we prefer people to propose code reviews through
113
Launchpad.
114
115
* <https://launchpad.net/+tour/code-review>
116
117
* <https://help.launchpad.net/Code/Review>
118
119
Anyone can propose or comment on a merge proposal just by creating a
120
Launchpad account.
121
122
There are two ways to create a new merge proposal: through the web
123
interface or by email.
124
125
126
Proposing a merge through the web
127
---------------------------------
128
129
To create the proposal through the web, first push your branch to Launchpad.
130
For example, a branch dealing with documentation belonging to the Launchpad
131
User mbp could be pushed as ::
132
133
bzr push lp:~mbp/bzr/doc
134
135
Then go to the branch's web page, which in this case would be
136
<https://code.launchpad.net/~mbp/bzr/doc>. You can simplify this step by just
137
running ::
138
139
bzr lp-open
140
141
You can then click "Propose for merging into another branch", and enter your
142
cover letter (see above) into the web form. Typically you'll want to merge
143
into ``~bzr/bzr/trunk`` which will be the default; you might also want to
144
nominate merging into a release branch for a bug fix. There is the option to
145
specify a specific reviewer or type of review, and you shouldn't normally
146
change those.
147
148
Submitting the form takes you to the new page about the merge proposal
149
containing the diff of the changes, comments by interested people, and
150
controls to comment or vote on the change.
151
152
Proposing a merge by mail
153
-------------------------
154
155
To propose a merge by mail, send a bundle to ``merge@code.launchpad.net``.
156
157
You can generate a merge request like this::
158
159
bzr send -o bug-1234.diff
160
161
``bzr send`` can also send mail directly if you prefer; see the help.
162
163
Reviewing changes
164
-----------------
165
166
From <https://code.launchpad.net/bzr/+activereviews> you can see all
167
currently active reviews, and choose one to comment on. This page also
168
shows proposals that are now approved and should be merged by someone with
169
PQM access.
170
Loggerhead is a web-based interface for Breezy
Version: 2.0.1