<!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="Content-Type" content="text/html; charset=utf-8" /> <title>Reviewing proposed changes to Bazaar — Bazaar 2.6.0 documentation</title> <link rel="stylesheet" href="_static/default.css" type="text/css" /> <link rel="stylesheet" href="_static/pygments.css" type="text/css" /> <script type="text/javascript"> var DOCUMENTATION_OPTIONS = { URL_ROOT: '', VERSION: '2.6.0', COLLAPSE_INDEX: false, FILE_SUFFIX: '.html', HAS_SOURCE: true }; </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> <link rel="shortcut icon" href="_static/bzr.ico"/> <link rel="top" title="Bazaar 2.6.0 documentation" href="index.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"> <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><a href="index.html">Developer Document Catalog (2.6.0)</a> »</li> </ul> </div> <div class="document"> <div class="documentwrapper"> <div class="bodywrapper"> <div class="body"> <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 <tt class="docutils literal"><span class="pre">bzr-loom</span></tt> 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 <tt class="docutils literal"><span class="pre">feed-pqm</span></tt> 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"> <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> <h3>This Page</h3> <ul class="this-page-menu"> <li><a href="_sources/code-review.txt" rel="nofollow">Show Source</a></li> </ul> <div id="searchbox" style="display: none"> <h3>Quick search</h3> <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> <p class="searchtip" style="font-size: 90%"> Enter search terms or a module, class or function name. </p> </div> <script type="text/javascript">$('#searchbox').show(0);</script> </div> </div> <div class="clearer"></div> </div> <div class="related"> <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><a href="index.html">Developer Document Catalog (2.6.0)</a> »</li> </ul> </div> <div class="footer"> © Copyright 2009-2011 Canonical Ltd. Created using <a href="http://sphinx.pocoo.org/">Sphinx</a> 1.1.3. </div> </body> </html>