Pull Request Guidelines

This document will describe a few guidelines to improves the chances that a pull request will be reviewed and merged into RTBKit.

Discuss

If you see a glaring flaw within RTBKit, resist the urge to jump into the code and make sweeping changes. I know it can be tempting but it's a wiser choice is to first discuss the proposed changes on the mailing list. It may turn out that someone is already working on this fix or that there's a good reason why that flaw exists. If nothing else, a discussion of the change will usually familiarize the reviewer with your proposed changes and streamline the review process when you finally create a pull request.

The depth of discussion that is necessary is relative to the size and scope of your changes. A good rule of thumb is to estimate how much time would be wasted if the pull request was rejected. If it's a couple of hours then you can probably dive head first and eat the loss in the worst case. Otherwise, making a quick check on the mailing list could save you lots of time down the line.

Scope

A pull request should contain a single logical change. This makes it much easier to review a pull request because it will usually reduce the amount of code that needs to be reviewed and each change can be evaluated independently. Packaging multiple changes in a single pull request is usually a bad idea because if any of the changes can't be merged then the reviewer is unlikely to cherry-pick individual changes and the whole pull request will be rejected.

A good rule of thumb is to create a separate branch in git whenever you make changes that you want to merge upstream. If a set of changes depend on another set of changes then you should create your new branch by forking off of the dependent branch which will cause Github to create a dependency between your pull requests.

Commits

Every commit associated with a pull request should have a descriptive title, build and pass all the tests. Beyond that, we currently put an emphasis on developer productivity so it's acceptable for development commits (eg. Fixed bug X in commit Y) to leak into the final pull request. As much as possible, try to rebase your branch on master instead of merging but we recognize that this is not always practical (eg. shared development branch) and pull requests will not be rejected because they contain merge commits.

Document

Your primary goal when documenting a pull request is to convince the reviewer that your changes significantly improve RTBKit. A good description will contain the following elements:

• A description of the problem. This is particularly important because it will determine how critical your pull request is.
• An overview of the proposed solution.
• Why this solution was chosen over other alternatives.
• How was the change tested.

While all these elements should be present, the size of each will heavily depend on the size of the proposed changes. A few lines is enough for a minor bug fix but a major new feature or refactoring will probably require several paragraphs.

To streamline any CLA related issues, it's useful if the reviewer can easily link your Github account to the name on the CLA. This can either be done by including your name within the pull request or sending an email directly to the reviewer. This should only be necessary for your first few accepted pull requests.

Follow up

While not ideal, pull requests do occasionally get forgotten. When this happens, a gentle reminder is usually needed to move the process along.