...

Submitting

...

Changes

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. Read the article "Using Pull Requests" on GitHub for further information.

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.however:

  • Especially if it’s blocking other work, it’s fine to ask someone to review, either by tagging them in a comment or asking on Slack or at the weekly tech call.

• 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.

  • See GitHub’s rebasing documentation and One Commit Per Pull Request for more information on rebasing.

When Reviewing a Pull Request

...

Reviewing and Merging Changes

We adopted Github's Pull Request Review for our repositories. Common checks that may occur in our repositories:

• A continous integration (CI) service, to ensure the test suite is passing (typically CircleCI or Travis)

• Rubocop, to be consistent w/ a baseline of Ruby style conventions

• Coveralls, to ensure code changes are covered by test changes

• Hound, to be consistent w/ Javascript and CSS style conventions

• 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?

...

If one or more of the required checks failed (or are incomplete), the code should not be merged (and the UI will not allow it). If all of the checks have passed, then anyone on the project (including the pull request submitter) may merge the code.

Things to Consider When Reviewing

First, the person contributing the code is putting themselves out there. Be mindful of what you say in a review.

• Ask clarifying questions

• State your understanding and expectations

• Provide example code or alternate solutions, and explain why

Things to consider:

• Does the commit message explain what is going on?

• Does the code changes have tests? Not all changes need new tests, some changes are refactorings

• Do new or changed methods, modules, and classes have documentation?

• 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? (Indicated by a cla-signed label in github.)

• Did the CI 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.

• Does the description of the new/changed specs match your understanding of what the spec is doing?

• Did the Continuous Integration tests complete successfully?

If you are uncertain, bring other contributors into the conversation by

...

assigning them as a reviewer.

Additional Resources

• General GitHub documentation

• GitHub pull request documentation

• Pro Git is both a free and excellent book about Git.

• A Git Config for Contributing