When Submitting a Pull Request
When you have code ready to be reviewed, and it meets the code guidelines, open a pull request to ask for it to be merged into the codebase.
To help make the review go smoothly, here are some general guidelines:
- Your pull request should address a single issue.
- It’s better to split large or complicated PRs into discrete steps if possible. This makes review more manageable and reduces the risk of conflicts with other changes.
- Give your pull request a brief title, and use the description to provide key information:
- Provide a list of the key changes
- If your PR addresses an existing ticket, link to it with “Fixes #123”. This will make it easy to refer back to the original ticket and automatically close it when the PR is merged.
- If you’ve discussed the issue, or just want to alert someone to your PR, tag them by including their @username.
- Link to relevant resources, such as Wiki pages, mailing list threads, specifications, or other tickets. This makes it easier to understand the full context of your PR.
- Please be patient. Your PR may not be reviewed right away, since the people doing code review are often busy, and may be traveling, in a different time zone, or otherwise not available to review your code immediately.
- Respond to code review comments, with discussion where it’s appropriate or by pushing additional commits to the branch. They will be added to the same PR automatically.
- If another PR is merged and conflicts with your changes, you may need to rebase your PR.
When Reviewing a Pull Request
Pull requests are evaluated in many different ways; fortunately, we have tools to help us with code review so that human review can focus on overall code quality and maintainability. For instance, we use:
- TravisCI, to ensure the test suite is passing
- Rubocop, to be consistent w/ a baseline of Ruby style conventions
- Coveralls, to ensure code changes are covered by test changes
- CodeClimate, to flag obvious code smells early for remediation
When reviewing a pull request, please take the time to review the changes and get a sense of what is being changed.
The key things to focus on are:
- Are the PR description and commit messages clear?
- Do the functional code changes match the PR description?
- Does the PR contain tests for new features or bugfixes?
- Not all PRs require tests, such as wording changes or simple refactoring.
- Does the commit contain more than it should? Are two separate concerns being addressed in one commit?
As a reviewer, it’s also your responsibility to make sure:
- Does the submitter have a signed CLA on file?
- Did the Travis tests complete successfully?
- Is there a significant drop in code coverage?
- Do all new or changed methods, modules, and classes have comments?
Merging Pull Requests
When merging pull requests:
- Give discussion time to settle down. When there is a contentious discussion, please allow 24 hours before merging to make sure everyone’s had a chance to respond.
- It is considered “poor form” to merge your own request, and you should be cautious when merging pull requests from other developers at your own insitution.
- If you are uncertain, bring other contributors into the conversation by creating a comment that includes their @username.
- If you like the pull request, but want others to chime in, create a +1 comment and tag a user.