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
- 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
- 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
- Don’t have permissions for the code
- Code is legally contentious
- Code is patented
- Code doesn’t include necessary copyright
- Not GDPR compliant
- 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
- 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
- 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 technology being introduced is not approved
- Outside technology being introduced is too costly monetarily