Pull Request Review Standards

Required for +1

Every PR should have an attached issue so that the PR reviewer knows what the intent was
PRs need to be small and focused - only one issue per PR
Don’t mix functional changes with style changes. If style changes, make a separate PR.
Design docs are required for a PR to be merged
- must link design doc in PR body
- for features but not for trivial PRs
If Feature Flag, then there must be a Design Doc. If Design Doc, then there must be a Feature Flag.
Tests are very important to look for in PRs. A couple things to do when looking at tests:
- If new low level functions, look for matching tests that are meaningful
- Commentary in the tests that describes what is being tested
- Helpful when debugging the test to know why the test was being written in the first place
- If not clear, ask for comments to tell a story
Unit tests are required for “hotspots” where we think there are the potential for hidden bugs
Architecture Design
- Coding best practices being followed
Do you understand this code and can you maintain it? If no, then no approval!
Must adhere to decisions from ADRs (including style)

If you are going to go into a PR, expect to spend some time on it. It is respect for the person who wrote the code.
Use SOLID design principles
Design Docs should be done before the PR rather than after
Make meaningful PR descriptions
- Short videos can be super useful
- Or screenshots!
Ask questions
Be positive
Be polite: avoid phrases like “do ___” - know that we are on the same team and we aren’t holding back your PRs just because we don’t like you - bringing suggestions that are better for the code medium and long term
When we bring suggestions, they might not necessarily come along with PR
One way of resolving a conversation is by creating a ticket for future work
Feedback is a gift