launchpad development|
Size: 4805
Comment:
|
Size: 4853
Comment: Reformat
|
| 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. Read this document carefully for a description of the review process and what it entails. We use Launchpad Code Merge Proposals to manage the queue of work. | The Launchpad code has been placed under mandatory pre-merge review. Read this document carefully for a description of the review process and what it entails. We use Launchpad Code Merge Proposals to manage the queue of work. |
| Line 10: | Line 13: |
| 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 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. |
| Line 15: | Line 22: |
| 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 '''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. |
| Line 17: | Line 29: |
| 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. | 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. |
| Line 22: | Line 36: |
| 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 RocketFuel. 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. | 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 RocketFuel. 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. |
| Line 27: | Line 46: |
| 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. | 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. |
| Line 32: | Line 56: |
| 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.) Reviewers will also check that your implementation has been discussed with a Launchpad staff developer in a pre-implementation call. | 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.) Reviewers will also check that your implementation has been discussed with a Launchpad staff developer in a pre-implementation call. |
| Line 37: | Line 66: |
| 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: | 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: |
| Line 39: | Line 71: |
| If what you're working on is complex enough that you'll likely be making several commits, you definitely need a branch. | If what you're working on is complex enough that you'll likely be making several commits, you definitely need a branch. |
| Line 46: | Line 79: |
| '''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 52: | Line 86: |
| 1. finding a reviewer (ask an on-call reviewer on #launchpad-reviews, or arrange with another reviewer) | 1. finding a reviewer (ask an on-call reviewer on #launchpad-reviews, or arrange with another reviewer) |
| Line 54: | Line 89: |
| 1. landing your change (You may land when the proposal is Approved) | 1. landing your change (You may land when the proposal is Approved) |
| Line 58: | Line 93: |
| * Reviewing requests in a timely manner, (respond to requests on #launchpad-reviews, ). | * Reviewing requests in a timely manner, (respond to requests on #launchpad-reviews, ). |
| Line 61: | Line 97: |
| * Set the status of your review, and if you are the final reviewer, set the status of the merge proposal. | * Set the status of your review, and if you are the final reviewer, set the status of the merge proposal. |
| Line 63: | Line 100: |
| 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. | 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. |
| Line 68: | Line 108: |
| Before you request a review, you should make sure that {{{make check_merge}}} reports no errors after having first merged rocketfuel^[1]^, as fixing any broken tests after a review might involve an unnecessary additional review which could have been avoided. | Before you request a review, you should make sure that {{{make check_merge}}} reports no errors after having first merged rocketfuel^[1]^, as fixing any broken tests after a review might involve an unnecessary additional review which could have been avoided. |
| Line 73: | Line 116: |
| A thorough guide to dealing with reviews and finally merging your changes is available at WorkingWithReviews. | A thorough guide to dealing with reviews and finally merging your changes is available at WorkingWithReviews. |
<<TableOfContents: execution failed [Too many arguments] (see also the log)>>
Introduction
The Launchpad code has been placed under mandatory pre-merge review. Read this document carefully for a description of the review process and what it entails. We use Launchpad Code Merge Proposals to manage the queue of work.
What are Pre-Merge Reviews?
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-Implementation Call
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.
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.
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 RocketFuel. 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
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.
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.) Reviewers will also check that your implementation has been discussed with a Launchpad staff developer in a pre-implementation call.
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.
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 reponsible for
- writing the code
- requesting review (use Launchpad Merge Proposals)
- finding a reviewer (ask an on-call reviewer on #launchpad-reviews, or arrange with another reviewer)
- dealing with review comments / requests (Reply to all queries)
- 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-reviews, ).
- Participating in follow-up discussion once a review is sent.
- 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.
Requesting review
Before you request a review, you should make sure that make check_merge reports no errors after having first merged rocketfuel[1], as fixing any broken tests after a review might involve an unnecessary additional review which could have been avoided.
Dealing with reviews
A thorough guide to dealing with reviews and finally merging your changes is available at WorkingWithReviews.
See also
The PythonStyleGuide
LaunchpadHackingFAQ has tips to keep your reviewer happy
The TestsStyleGuide
