How to Review Pull Requests

How to Review Pull Requests

This is a checklist style approach to reviewing PRs in Starchart or Telescope. It is based on the excellent code review guidelines at Google. Google's "senior" rule for code review is also applicable to us:

Consider approving a change once it is definitely improving the overall code health, even if it isn’t perfect--you can iterate on it later. “There is no such thing as perfect code, only better code.”

We don't want to block work because it isn't perfect. Ask the developer to file follow-up Issues (or do it yourself) and accept anything that moves the project forward, even if imperfectly. We can and will do further fixes later on.

Guidelines for Review

1. Examine the PR's structure. Is it done correctly? Does the title reflect what it's about? Is the description formatted correctly (e.g., "Closes #123", includes relevant sections)? If not, fix it, or ask the author to fix it.
2. Read the PR's content. Does the description clearly discuss what this is fixing? Can you understand the problem, goals, and implementation? Is it clear what was changed? Ask questions for clarification if things aren't clear or obvious to you.
3. Do we want to accept this PR? Is it necessary? Just because someone submits a fix or feature doesn't mean we need to take it. If the answer is 'no', can anything be done to change that to 'yes'? How will we work with the developer to deal with this?
4. Confirm that CI is passing. Does the code pass linting, builds, unit tests, preview builds, etc. on all platforms? If it does not, figure out why, and fail the PR with a comment indicating why, and how to address things.
5. Make sure there are proper instructions for testing this change in the description. If you aren't clear how to run and test the fix or feature, ask for clarification. The PR author should help you figure this out.
6. Test the PR yourself. This might involve running the PR locally. The fix or feature should solve the problem it was meant to address.
7. Test edge cases. Does the solution work in a limited way, but break when you try it in some other context? For example, a frontend fix might work on desktop, but break on mobile. A backend change might break when run in Docker (i.e. production), but work on the author's machine. Try to exercise the fix in different ways to make sure that it does what it says, and doesn't regress or fail other parts of the code. If there are odd edge case failures, consider whether they need to be fixed in this PR or in a follow-up Issue. File new Issues for things that don't need to block this fix from landing.
8. Examine the PR's code. Is it addressing the problem specified in the description? Is it touching files/lines unrelated to this fix (don't allow this to happen)? Could it be broken up into smaller pieces (e.g., separate PRs)? A change of a few hundred lines is OK, thousands of lines may not be. Does it included commented-out/dead code (it shouldn't)? Does it delete or add things unrelated to this fix?
9. Read the PR's code line-by-line. Do you understand what every line is doing? If you don't, that's not your problem, that's our problem and we need to fix it. Ask questions about lines that aren't clear (e.g., "need comments to clarify," or "this is overly clever and could be simplified," or "this uses a bad coding practice," etc). Could we simplify or clean anything up? Good code is simple code, because anyone can maintain it.
10. Check for tests. Does the PR include tests? Is it testable at all? Could we add them if they are missing? Do we need them to be added in this PR or could they be done in a follow-up PR? If the tests are here, read them just like the rest of the code and make sure they aren't brittle and likely to break later on.
11. Do we need to update our docs based on this change? Does this fix introduce a new workflow for developers? Does it change things for users? How will we capture this knowledge and communicate it to the rest of the team and/or userbase?
12. Who else should review this code? Perhaps you are only comfortable doing some portion of the steps above. Are there other team members who could focus on specific aspects? For example, does it affect design, deployment, security, or some other area, where an expert should be called in? Find the right people to help you review this. Likely we need 2, 3, or more people looking at a PR.
13. Is the code needing to be squashed and rebased? Are the commits a mess and needing to be cleaned up? Before we merge work, we want a single commit that is rebased on the default branch.
14. Is the original developer willing to work with you to correct any issues in the PR? If not, who should take over this work?