Looking at things from a new perspective is always an enriching experience. I have not worked with PRs for half a year now (we do Trunk Based Development), so I have been thinking about them a lot.
Here are some thoughts about what PRs are good for, and what they are dreadful for.
What PRs are for
Alignment towards a goal
All PRs are steps towards a goal – even the huge ones. That’s why reviewers are responsible to ensure each step drives the system in the goal’s direction. PRs should be designed around explaining why this is an incremental step, not what is being committed (surely the reviewer can read the code for that).
Highlighting bugs
It’s not the reviewer’s responsibility to highlight bugs in the code. They are humans, thus error prone. Automated tests are the tool you want to use. PRs are a good checkpoint to automatically running those and verifying that the changes have integrated well with old code, while adding new tests for new behaviour.
Discussing quality standards
Every team will have their alignment on their code style and quality standards. PRs are a great opportunity to check that things are aligned with the shared definition of the team and evangelize when there are some things to make better.
What they are NOT for
Human linting
For the same reason reviewers shouldn’t be the last line of defense on bugs, they shouldn’t be accountable for linting your code as well. Static linters are fast, deterministic, and more up-to-date with best practices. Don’t expect your reviewer to find that typo or that unused import. Integrate linting tools to your development experience (most IDEs do it for free) and have it enforced as automation in your PR automations.
Enforcing one’s opinion on how to do something
Some people find the power in blocking a PR an opportunity to put their thoughts down someone else’s throat. You shouldn’t block a PR UNLESS you see some major problem in its quality or the features it enables.
On quality, I think this always has to be thought in context. Maintainability is key for code health, but so is evolving design through refactoring. Quality doesn’t mean having the perfect solution before unblocking, but to have enough seams in the code to refactor things easily.
Being a smart ass
Yes, you’re a genius. Congratulations! Now find an audience who actually cares about your lectures and let people work!
Learning about a feature / goal
User stories are meant for this. Keep the conversation around feedback. If you need context, ask for it somewhere else – people should be able to read the PR as documentation on why the changes were introduced, not on the full story.
Refining requirements
Doubts on what needs to be achieved? Stop! Do this with the right people in the right meeting. If you’re writing PRs with uncertainty (unless it’s a throwaway POC), you’re in the wrong step of the process. Go back to a room and refine those requirements further.
My model for a perfect PR flow
I’ve been imagining how to combine the fast feedback loops of Pair programming, the continuous integration of Trunk Based Development, and the more independent nature of PRs.
Here’s my take on what the ideal would look like:
-
Refine requirements and align on the story with several engineers: Stories are clearly understood and the goal is set in simple terms. Acceptance criteria (and so acceptance tests) are agreed.
-
Create a new feature branch from trunk.
-
Split into the smallest steps possible: Small steps are key for fast feedback and adaptation to change.
-
Create one or multiple PRs to the Feature Branch for each step: Parallelize when possible. Require reviews from at least 2 engineers with context on the goal that the feature tries to achieve.
-
Integrate feature branch with trunk at least once a day: This is key to reduce uncertainty on the final integration with trunk. If feature branch diverges from trunk, stop development of the feature and fix it.
-
Squash feature branch into trunk when all work is done: Intermediate steps are relevant only to the development process. Squash them into a simple, story-related commit that can explain why the feature was implemented in the future.