Code Review Process

Owned by Heather Greer Klein
Last updated: Dec 17, 2024
3 min read

Following is general guidance for the code review process within the Samvera Community. For guidance specific to a repository (like Hyrax or Hyku), look for information on how to contribute in that repository on the Samvera Github.

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

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

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?

  • 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