Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?

Common Reasons Reviewers Block a PR

Intro

Each team has their own tolerance for what is and is not a reason to reject a PR. This may be rooted in process, fairness, and expediency, and may be a company or team decision. Below is a list of reasons you might want to reject a PR. As a team, you can move through the checklist and decide your own reasons for rejecting a PR. Remember too, that there are multiple ways to handle PRs and your team may be more comfortable with comments and conversations than rejections. It’s up to each team’s individual style

User-facing reasons for rejection

  • Breaks the app
  • Fails to compile
  • Introduces a bug
  • Is likely to introduce a bug in the future
  • Is likely to allow a bug to be introduced in the future
  • Does not fully implement story
  • Does not display correctly
  • Does not display correctly on all platforms/browsers
  • Adds in unnecessary functionality not approved. “You ain’t gonna need it.” (YAGNI)
  • PR does not consider migration from existing customer to new feature
  • Will slow down the user experience — i.e. N+1 query, non-performant code
  • Does not consider certain edge cases
  • Is not i18n compatible

Security

  • Introduces a security issue
  • Could introduce a security issue
  • Privacy concerns, such as storing personally identifiable information (PII) unnecessarily or without proper authority, not properly securing PII, and accidentally logging PII in the system

Legal

  • Don’t have permissions for the code
  • Code is legally contentious
  • Code is patented
  • Code doesn’t include necessary copyright
  • Not GDPR compliant

Process

  • Changeset is too large and needs to be decomposed into smaller PRs which could be considered independently.
  • PR fails to have accompanying issue in issue tracker
  • Failure to document what the PR is doing and why
  • Wrong timing, needs other work to be done in the app first

Testing

  • Lack of tests
  • Tests fail to test feature
  • Introduces a test that takes too much time for not enough value
  • Tests implemented incorrectly (implemented an integration test inside a unit test framework)
  • Breaks tests

Style & Communication

  • File or function is too long
  • Code is incorrect
  • Non-descriptive naming
  • Logic not clear from reading the PR
  • Functionality is already implemented elsewhere in codebase
  • Needs greater legibility
  • Unlikely that someone could fix a bug in this code due to legibility
  • Introduces too much technical debt
  • Is not DRY enough, needs a refactor
  • Too DRY, optimizing in advance and may be difficult to change later
  • Lack of documentation

Outside Technologies

  • Outside technology being introduced is not approved
  • Outside technology being introduced is too costly monetarily
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.