/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-review.txt

  • Committer: Robert Collins
  • Date: 2007-04-19 02:27:44 UTC
  • mto: This revision was merged to the branch mainline in revision 2426.
  • Revision ID: robertc@robertcollins.net-20070419022744-pfdqz42kp1wizh43
``make docs`` now creates a man page at ``man1/bzr.1`` fixing bug 107388.
(Robert Collins)

Show diffs side-by-side

added added

removed removed

Lines of Context:
doc/developers/code-review.txt
1
 
Reviewing proposed changes to Breezy
2
 
####################################
3
 
 
4
 
All non-trivial code changes coming in to Breezy are reviewed by someone else.
5
 
 
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.
9
 
 
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.
13
 
 
14
 
No one likes their merge requests sitting in a queue going nowhere: this
15
 
is pure waste.  We prioritize reviewing existing proposals.
16
 
 
17
 
We do all our code reviews through Launchpad's merge proposal interface.
18
 
 
19
 
 
20
 
Reviewing proposed changes
21
 
==========================
22
 
 
23
 
There are three main requirements for code to get in:
24
 
 
25
 
* Doesn't reduce test coverage: if it adds new methods or commands,
26
 
  there should be tests for them.  There is a good test framework
27
 
  and plenty of examples to crib from, but if you are having trouble
28
 
  working out how to test something feel free to post a draft patch
29
 
  and ask for help.
30
 
 
31
 
* Doesn't reduce design clarity, such as by entangling objects
32
 
  we're trying to separate.  This is mostly something the more
33
 
  experienced reviewers need to help check.
34
 
 
35
 
* Improves bugs, features, speed, or code simplicity.
36
 
 
37
 
Code that goes in should not degrade any of these aspects.  Patches are
38
 
welcome that only cleanup the code without changing the external
39
 
behaviour.  The core developers take care to keep the code quality high
40
 
and understandable while recognising that perfect is sometimes the enemy
41
 
of good.
42
 
 
43
 
It is easy for reviews to make people notice other things which should be
44
 
fixed but those things should not hold up the original fix being accepted.
45
 
New things can easily be recorded in the bug tracker instead.
46
 
 
47
 
It's normally much easier to review several smaller patches than one large
48
 
one.  You might want to submit a preparatory patch that will make your "real"
49
 
change easier.
50
 
 
51
 
 
52
 
Checklist for reviewers
53
 
=======================
54
 
 
55
 
* Do you understand what the code's doing and why?
56
 
 
57
 
* Will it perform reasonably for large inputs, both in memory size and
58
 
  run time?  Are there some scenarios where performance should be
59
 
  measured?
60
 
 
61
 
* Is it tested, and are the tests at the right level?  Are there both
62
 
  blackbox (command-line level) and API-oriented tests?
63
 
 
64
 
* If this change will be visible to end users or API users, is it
65
 
  appropriately documented in release notes and/or in whats-new ?
66
 
 
67
 
* Does it meet the `coding standards <code-style.html>`_?
68
 
 
69
 
* If it changes the user-visible behaviour, does it update the help
70
 
  strings and user documentation?
71
 
 
72
 
* If it adds a new major concept or standard practice, does it update the
73
 
  developer documentation?
74
 
 
75
 
* (your ideas here...)
76
 
 
77
 
 
78
 
Reviews on Launchpad
79
 
====================
80
 
 
81
 
Anyone can propose or comment on a merge proposal just by creating a
82
 
Launchpad account.
83
 
 
84
 
From <https://code.launchpad.net/brz/+activereviews> you can see all
85
 
currently active reviews, and choose one to comment on.  This page also
86
 
shows proposals that are now approved and should be merged by someone with
87
 
PQM access.
88
 
 
89
 
<https://help.launchpad.net/Code/Review> explains the various merge proposal
90
 
states.  Note that we don't use state *Approved* until the patch is completely
91
 
ready to merge.
92
 
 
93
 
 
94
 
Landing approved changes
95
 
========================
96
 
 
97
 
Once a merge proposal is approved and finished, it's marked as
98
 
Approved and picked up by the CI (https://ci.breezy-vcs.org/),
99
 
which will automatically test and integrate it.