Diff for "PreMergeReviews"

Not logged in - Log In / Register

Differences between revisions 8 and 16 (spanning 8 versions)
Revision 8 as of 2010-02-19 21:20:11
Size: 4854
Editor: flacoste
Comment: typo
Revision 16 as of 2021-11-11 10:00:34
Size: 6259
Editor: cjwatson
Comment: Move Python style guide to RTD
Deletions are marked like this. Additions are marked like this.
Line 5: Line 5:
The Launchpad code has been placed under mandatory pre-merge review. All changes to Launchpad need to have a merge proposal discussing the change.
Line 7: Line 7:
what it entails. We use Launchpad Code Merge Proposals to manage the
queue of work
.
what it entails. We use Launchpad Code Merge Proposals to manage this process.
Line 13: Line 11:
Pre-merge reviews are peer reviews of source code modifications. They
are a great tool for ensuring quality of the resulting product, and also
for communicating technique and style between team members. A good
review process ensures that everybody learns at a much faster rate, and
creates a valuable technical relationship between reviewer and coder.
Pre-merge reviews are peer reviews of source code modifications. They are one tool for finding defects in code changes to Launchpad. Some knowledge sharing and tips n tricks can be shared via code review as well, though that is a secondary (and low frequency) effect. We do code reviews because it is easy when making non trivial (or even trivial) changes to miss something that a second set of eyeballs will catch. This is particularly the case in some areas of Launchpad which have deep and unobvious coupling.
Line 22: Line 16:
We '''require''' all comunity developers to arrange pre-implementation
design reviews with a Launchpad staff developer from the relevant team
or with significant domain knowledge. Pre-implementation reviews should
preferably happen by phone call but IRC can be used if a phone call is
not possible. Launchpad staff developers are also encouraged to have
pre-implementation reviews with another staff developer.
We recommend that new contributors chat to an experienced developer before
committing a lot of time to a change: often a small amount of discussion
can make the coding time much shorter and less likely to run into friction
within the Launchpad code base or product needs.
Line 29: Line 21:
Pre-implementation calls should be used before starting to implement any
feature or fix any bug. For more details on the work flow and what is
expected from both callers and callees, see DesignPhoneCall.
If desired, a voice call can be held (e.g. over mumble/skype/voip/pots) - the extra bandwidth of such calls can help avoid misunderstandings and get to the bottom of things much more quickly.
Line 33: Line 23:
Experienced developers are also able to have such pre implementation calls - and its particularly good to do them when working on an area of the code base they are not already familiar with.

There is a semi formal process for these calls, but it is rarely used. For details see DesignPhoneCall.
Line 39: Line 32:
code into RocketFuel. The sections below go into the details of how this code into [[../Trunk|Trunk]]. The sections below go into the details of how this
Line 46: Line 39:
A limit of 800 lines per branch diff has been set as a maximum. If your
branch grows above this limit you must negotiate with a reviewer to
accept the oversized branch. Note that very large branches are not
recommended for on-call reviews as they slow down the work of the
reviewer who is trying interactively review a high volume of branches
during the shift.
Complex changes are harder to review and more likely to let harmful changes into the code base. We use a limit of 800 lines for the diff of a branch as a proxy metric for complexity: if the diff of your branch on the merge proposal page has more than 800 lines a reviewer may well ask you to break it into smaller pieces for targeted review. Note that 800 lines is simple a proxy metric: any reviewer can suggest that an overly complex change be split into multiple simpler steps - even if the line count in the diff is much smaller than 800. Conversely much longer branches are ok if they are actually simple : the reviewer and proposer need to talk about such cases.
Line 59: Line 46:
cheat-sheet to do a self-review.) Reviewers will also check that your
implementation has been discussed with a Launchpad staff developer in a
pre-implementation call.
cheat-sheet to do a self-review.)
Line 63: Line 48:
== Use a branch ==
Line 64: Line 50:
== If in doubt, use a branch ==

Any non-trivial changes for review must be on a branch. The goal is to
make it easy to look at which changes have been made without spurious
diffs, and without having to worry about RocketFuel changing from under
your feet. A good rule of thumb is:

If what you're working on is complex enough that you'll likely be making
several commits, you definitely need a branch.
All changes going into Launchpad require a branch with the change(s) on it, and a merge proposal proposing those changes and explaining what the change wants to accomplish, why its desired and how it goes about it.
Line 79: Line 56:
  '''The coder is responsible for getting his branch through the review
process'''
  '''The coder is responsible for getting his branch through the review process'''
Line 86: Line 62:
  1. finding a reviewer (ask an on-call reviewer on #launchpad-reviews,
 
or arrange with another reviewer)
  1. dealing with review comments / requests (Reply to all queries)
  1. finding a reviewer (ask an on-call reviewer on #launchpad-dev, or arrange with another reviewer)
  1. dealing with review comments / requests
Line 94: Line 69:
  #launchpad-reviews, ).   #launchpad-dev, review proposals on [[https://code.launchpad.net/launchpad/+activereviews|+activereviews]]).
Line 96: Line 71:
  * Be clear about the changes you expect to see
  * Set the status of your review, and if you are the final reviewer,
  set the status of the merge proposal.

The reviewer is the obvious bottleneck in the process, and if you can
make his life easier, please do. This includes making smaller changes,
coordinating with other developers to order reviews to minimize
conflicts, and reminding of forgotten branches periodically.
  * Be clear about things you are:
    * suggesting
    * requiring
    * asking for discussion or consideration
  * Set the status of your review
  * Set the status of the merge proposal (unless other requested reviews are outstanding)
Line 109: Line 81:
{{{make check_merge}}} reports no errors after having first merged
rocketfuel^[1]^, as fixing any broken tests after a review might
{{{make check}}} reports no errors after having first merged
trunk, as fixing any broken tests after a review might
Line 113: Line 85:
== Choosing to self review ==

If you are a '''reviewer''' then you can choose to review your own code under some conditions.

Not all changes benefit from code review / are high enough risk to need formal peer review. For instance (not exhaustive):
 * mechanical things (like moving code)
 * updating source deps
 * rollbacks
 * typo fixes
 * improvements to documentation

For many of these things a review may add value - but less value than doing the review uses up. We want reviews to be a net win for the effort being put into developing Launchpad, and so we should, once we're comfortable people know how things should be, allow them to decide if a particular change is an improvement on its own, or an improvement that also /needs/ other input before landing.

Becoming a fully fledged reviewer is the trigger that marks our comfort that a developer can make this assessment effectively.

There are some parts of the system that we do not currently permit self review: UI changes.

To do a self review, do exactly the same thing as normal, but claim the review yourself and vote approve.

To do a self review of database changes, you need to be a database reviewer.
Line 122: Line 114:
 * The PythonStyleGuide  * The [[https://launchpad.readthedocs.io/en/latest/guides/python.html|Python style guide]]

<<TableOfContents: execution failed [Too many arguments] (see also the log)>>

Introduction

All changes to Launchpad need to have a merge proposal discussing the change. Read this document carefully for a description of the review process and what it entails. We use Launchpad Code Merge Proposals to manage this process.

What are Pre-Merge Reviews?

Pre-merge reviews are peer reviews of source code modifications. They are one tool for finding defects in code changes to Launchpad. Some knowledge sharing and tips n tricks can be shared via code review as well, though that is a secondary (and low frequency) effect. We do code reviews because it is easy when making non trivial (or even trivial) changes to miss something that a second set of eyeballs will catch. This is particularly the case in some areas of Launchpad which have deep and unobvious coupling.

Pre-Implementation Call

We recommend that new contributors chat to an experienced developer before committing a lot of time to a change: often a small amount of discussion can make the coding time much shorter and less likely to run into friction within the Launchpad code base or product needs.

If desired, a voice call can be held (e.g. over mumble/skype/voip/pots) - the extra bandwidth of such calls can help avoid misunderstandings and get to the bottom of things much more quickly.

Experienced developers are also able to have such pre implementation calls - and its particularly good to do them when working on an area of the code base they are not already familiar with.

There is a semi formal process for these calls, but it is rarely used. For details see DesignPhoneCall.

Working with PreMergeReviews

For pre-merge reviews to be effective, however, small modifications to the development workflow are required. The main modification is, of course, requesting peer review of your code changes before merging your code into Trunk. The sections below go into the details of how this is best performed. There is a crib sheet for getting a branch merged on the WorkingWithReviews page.

Branch size limits

Complex changes are harder to review and more likely to let harmful changes into the code base. We use a limit of 800 lines for the diff of a branch as a proxy metric for complexity: if the diff of your branch on the merge proposal page has more than 800 lines a reviewer may well ask you to break it into smaller pieces for targeted review. Note that 800 lines is simple a proxy metric: any reviewer can suggest that an overly complex change be split into multiple simpler steps - even if the line count in the diff is much smaller than 800. Conversely much longer branches are ok if they are actually simple : the reviewer and proposer need to talk about such cases.

What are reviewers looking for?

Reviewers are checking both the design of your branch and the quality of the code in it. The best way to understand what a review is looking for is to read TipsForReviewers. (You can also use the CodeReviewChecklist cheat-sheet to do a self-review.)

Use a branch

All changes going into Launchpad require a branch with the change(s) on it, and a merge proposal proposing those changes and explaining what the change wants to accomplish, why its desired and how it goes about it.

You are responsible for your branch

The central rule that is used in the Launchpad review process is:

  • The coder is responsible for getting his branch through the review process

This means that you, as a coder, are responsible for

  1. writing the code
  2. requesting review (use Launchpad Merge Proposals)
  3. finding a reviewer (ask an on-call reviewer on #launchpad-dev, or arrange with another reviewer)
  4. dealing with review comments / requests
  5. landing your change (You may land when the proposal is Approved)

The reviewer, conversely, is responsible only for two important things:

  • Reviewing requests in a timely manner, (respond to requests on

    #launchpad-dev, review proposals on +activereviews).

  • Participating in follow-up discussion once a review is sent.
  • Be clear about things you are:
    • suggesting
    • requiring
    • asking for discussion or consideration
  • Set the status of your review
  • Set the status of the merge proposal (unless other requested reviews are outstanding)

Requesting review

Before you request a review, you should make sure that make check reports no errors after having first merged trunk, as fixing any broken tests after a review might involve an unnecessary additional review which could have been avoided.

Choosing to self review

If you are a reviewer then you can choose to review your own code under some conditions.

Not all changes benefit from code review / are high enough risk to need formal peer review. For instance (not exhaustive):

  • mechanical things (like moving code)
  • updating source deps
  • rollbacks
  • typo fixes
  • improvements to documentation

For many of these things a review may add value - but less value than doing the review uses up. We want reviews to be a net win for the effort being put into developing Launchpad, and so we should, once we're comfortable people know how things should be, allow them to decide if a particular change is an improvement on its own, or an improvement that also /needs/ other input before landing.

Becoming a fully fledged reviewer is the trigger that marks our comfort that a developer can make this assessment effectively.

There are some parts of the system that we do not currently permit self review: UI changes.

To do a self review, do exactly the same thing as normal, but claim the review yourself and vote approve.

To do a self review of database changes, you need to be a database reviewer.

Dealing with reviews

A thorough guide to dealing with reviews and finally merging your changes is available at WorkingWithReviews.

See also

PreMergeReviews (last edited 2021-11-11 10:00:34 by cjwatson)