<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="X-UA-Compatible" content="IE=Edge" />
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Reviewing proposed changes to Bazaar — Bazaar 2.7.0 documentation</title>
<link rel="stylesheet" href="_static/classic.css" type="text/css" />
<link rel="stylesheet" href="_static/pygments.css" type="text/css" />
<script type="text/javascript" id="documentation_options" data-url_root="./" src="_static/documentation_options.js"></script>
<script type="text/javascript" src="_static/jquery.js"></script>
<script type="text/javascript" src="_static/underscore.js"></script>
<script type="text/javascript" src="_static/doctools.js"></script>
<script type="text/javascript" src="_static/language_data.js"></script>
<link rel="shortcut icon" href="_static/bzr.ico"/>
<link rel="search" title="Search" href="search.html" />
<link rel="next" title="Bazaar Code Style Guide" href="code-style.html" />
<link rel="prev" title="Bazaar Testing Guide" href="testing.html" />
<link rel="stylesheet" href="_static/bzr-doc.css" type="text/css" />
</head><body>
<div class="related" role="navigation" aria-label="related navigation">
<h3>Navigation</h3>
<ul>
<li class="right" style="margin-right: 10px">
<a href="code-style.html" title="Bazaar Code Style Guide"
accesskey="N">next</a></li>
<li class="right" >
<a href="testing.html" title="Bazaar Testing Guide"
accesskey="P">previous</a> |</li>
<li><a href="http://bazaar.canonical.com/">
<img src="_static/bzr icon 16.png" /> Home</a> | </li>
<a href="http://doc.bazaar.canonical.com/en/">Documentation</a> | </li>
<li class="nav-item nav-item-0"><a href="index.html">Developer Document Catalog (2.7.0)</a> »</li>
</ul>
</div>
<div class="document">
<div class="documentwrapper">
<div class="bodywrapper">
<div class="body" role="main">
<div class="section" id="reviewing-proposed-changes-to-bazaar">
<h1>Reviewing proposed changes to Bazaar<a class="headerlink" href="#reviewing-proposed-changes-to-bazaar" title="Permalink to this headline">¶</a></h1>
<p>All non-trivial code changes coming in to Bazaar are reviewed by someone else.</p>
<p>Anyone is welcome to review any patch. You don’t need to have a full
understanding of the codebase to find problems in the code, the documentation,
or the concept of the patch.</p>
<p>Normally changes by core contributors are reviewed by one other core
developer, and changes from other people are reviewed by two core
developers. Use intelligent discretion about whether the patch is trivial.</p>
<p>No one likes their merge requests sitting in a queue going nowhere: this
is pure waste. We prioritize reviewing existing proposals.
Canonical dedicates some staff time to providing prompt helpful reviews.
(See <<a class="reference external" href="http://wiki.bazaar.canonical.com/PatchPilot/">http://wiki.bazaar.canonical.com/PatchPilot/</a>>.)</p>
<p>From late 2009 on, we do all our code reviews through Launchpad’s
merge proposal interface.</p>
<div class="section" id="reviewing-proposed-changes">
<h2>Reviewing proposed changes<a class="headerlink" href="#reviewing-proposed-changes" title="Permalink to this headline">¶</a></h2>
<p>There are three main requirements for code to get in:</p>
<ul class="simple">
<li>Doesn’t reduce test coverage: if it adds new methods or commands,
there should be tests for them. There is a good test framework
and plenty of examples to crib from, but if you are having trouble
working out how to test something feel free to post a draft patch
and ask for help.</li>
<li>Doesn’t reduce design clarity, such as by entangling objects
we’re trying to separate. This is mostly something the more
experienced reviewers need to help check.</li>
<li>Improves bugs, features, speed, or code simplicity.</li>
</ul>
<p>Code that goes in should not degrade any of these aspects. Patches are
welcome that only cleanup the code without changing the external
behaviour. The core developers take care to keep the code quality high
and understandable while recognising that perfect is sometimes the enemy
of good.</p>
<p>It is easy for reviews to make people notice other things which should be
fixed but those things should not hold up the original fix being accepted.
New things can easily be recorded in the bug tracker instead.</p>
<p>It’s normally much easier to review several smaller patches than one large
one. You might want to use <code class="docutils literal notranslate"><span class="pre">bzr-loom</span></code> to maintain threads of related
work, or submit a preparatory patch that will make your “real” change
easier.</p>
</div>
<div class="section" id="checklist-for-reviewers">
<h2>Checklist for reviewers<a class="headerlink" href="#checklist-for-reviewers" title="Permalink to this headline">¶</a></h2>
<ul class="simple">
<li>Do you understand what the code’s doing and why?</li>
<li>Will it perform reasonably for large inputs, both in memory size and
run time? Are there some scenarios where performance should be
measured?</li>
<li>Is it tested, and are the tests at the right level? Are there both
blackbox (command-line level) and API-oriented tests?</li>
<li>If this change will be visible to end users or API users, is it
appropriately documented in release notes and/or in whats-new ?</li>
<li>Does it meet the <a class="reference external" href="code-style.html">coding standards</a>?</li>
<li>If it changes the user-visible behaviour, does it update the help
strings and user documentation?</li>
<li>If it adds a new major concept or standard practice, does it update the
developer documentation?</li>
<li>(your ideas here…)</li>
</ul>
</div>
<div class="section" id="reviews-on-launchpad">
<h2>Reviews on Launchpad<a class="headerlink" href="#reviews-on-launchpad" title="Permalink to this headline">¶</a></h2>
<p>Anyone can propose or comment on a merge proposal just by creating a
Launchpad account.</p>
<p>From <<a class="reference external" href="https://code.launchpad.net/bzr/+activereviews">https://code.launchpad.net/bzr/+activereviews</a>> you can see all
currently active reviews, and choose one to comment on. This page also
shows proposals that are now approved and should be merged by someone with
PQM access.</p>
<p><<a class="reference external" href="https://help.launchpad.net/Code/Review">https://help.launchpad.net/Code/Review</a>> explains the various merge proposal
states. Note that we don’t use state <em>Approved</em> until the patch is completely
ready to merge.</p>
</div>
<div class="section" id="landing-approved-changes">
<h2>Landing approved changes<a class="headerlink" href="#landing-approved-changes" title="Permalink to this headline">¶</a></h2>
<p>Once a merge proposal is approved and finished, it’s sent to PQM (the patch
queue manager) which will automatically test and integrate it. The recommended
way to start this off is by running the <code class="docutils literal notranslate"><span class="pre">feed-pqm</span></code> script from
<<a class="reference external" href="https://launchpad.net/hydrazine/">https://launchpad.net/hydrazine/</a>>.</p>
</div>
</div>
</div>
</div>
</div>
<div class="sphinxsidebar" role="navigation" aria-label="main navigation">
<div class="sphinxsidebarwrapper">
<h3><a href="index.html">Table of Contents</a></h3>
<ul>
<li><a class="reference internal" href="#">Reviewing proposed changes to Bazaar</a><ul>
<li><a class="reference internal" href="#reviewing-proposed-changes">Reviewing proposed changes</a></li>
<li><a class="reference internal" href="#checklist-for-reviewers">Checklist for reviewers</a></li>
<li><a class="reference internal" href="#reviews-on-launchpad">Reviews on Launchpad</a></li>
<li><a class="reference internal" href="#landing-approved-changes">Landing approved changes</a></li>
</ul>
</li>
</ul>
<h4>Previous topic</h4>
<p class="topless"><a href="testing.html"
title="previous chapter">Bazaar Testing Guide</a></p>
<h4>Next topic</h4>
<p class="topless"><a href="code-style.html"
title="next chapter">Bazaar Code Style Guide</a></p>
<div role="note" aria-label="source link">
<h3>This Page</h3>
<ul class="this-page-menu">
<li><a href="_sources/code-review.txt"
rel="nofollow">Show Source</a></li>
</ul>
</div>
<div id="searchbox" style="display: none" role="search">
<h3>Quick search</h3>
<div class="searchformwrapper">
<form class="search" action="search.html" method="get">
<input type="text" name="q" />
<input type="submit" value="Go" />
<input type="hidden" name="check_keywords" value="yes" />
<input type="hidden" name="area" value="default" />
</form>
</div>
</div>
<script type="text/javascript">$('#searchbox').show(0);</script>
</div>
</div>
<div class="clearer"></div>
</div>
<div class="related" role="navigation" aria-label="related navigation">
<h3>Navigation</h3>
<ul>
<li class="right" style="margin-right: 10px">
<a href="code-style.html" title="Bazaar Code Style Guide"
>next</a></li>
<li class="right" >
<a href="testing.html" title="Bazaar Testing Guide"
>previous</a> |</li>
<li><a href="http://bazaar.canonical.com/">
<img src="_static/bzr icon 16.png" /> Home</a> | </li>
<a href="http://doc.bazaar.canonical.com/en/">Documentation</a> | </li>
<li class="nav-item nav-item-0"><a href="index.html">Developer Document Catalog (2.7.0)</a> »</li>
</ul>
</div>
<div class="footer" role="contentinfo">
© Copyright 2009-2011 Canonical Ltd.
Created using <a href="http://sphinx-doc.org/">Sphinx</a> 1.8.4.
</div>
</body>
</html>
