“The ratio of time spent reading versus writing is well over 10 to 1. We are constantly reading old code as part of the effort to write new code. …[Therefore,] making it easy to read makes it easier to write.” ― Robert C. Martin, Clean Code: A Handbook of Agile Software Craftsmanship
Pull Requests
With git, we use a mechanism called “Pull Requests” to review code before it reaches master. PR’s are primarily to promote team communication about code. We can learn from each other’s code We can help each other to not repeat similar patterns/mistakes The goal is to collaborate on making something higher quality than what any single engineer could have produced themselves. Make no mistake though, reviewing PR’s is hard. Feel free to comment and improve this manifesto to make it work better for all of us!
The Pull Request Manifesto
As a Pull Request writer I will:
Review my own code first
As a PR writer I want to minimize silly mistakes.
Before opening a PR up for review, read through it yourself. Don’t just check for mistakes — imagine reading the code for the first time. What might confuse you?
Answer questions with the code itself
As a PR writer I want my code to be self-documenting and self-explanatory.
If you have code that has people asking for clarification during review, rather than just explaining it there in the review, change the code to capture that explanation more clearly.
Keep PR’s as small as I can As a PR writer I want to have short review cycles.
Large PR’s mean:
- longer reviews
- it’s more likely that mistakes will be over-looked
- it’s more likely that you will have merge/conflict/rebase issues
Use comments in my own PR’s to guide the reviewer As a PR writer I want to
- document my reasoning
- draw attention to complex areas
- draw attention to areas I am unsure about to get the best focus and feedback in those areas.
- use a visual if possible (e.g. if it’s front end, use a screenshot for example)
Not take feedback personally As A PR writer I will accept all feedback as good feedback.
You should receive feedback given on a PR with good intent, with an aim at continuous improvement. If you don’t understand what the context of the feedback is, ask the reviewer!
Reach out to possible PR reviewers As a PR writer it is my responsibility to encourage people to review my pull requests.
Reviewing pull requests takes time and it is often difficult to overlook your PR among the many things going on in your colleagues’ day. Take the initiative and ask others (nicely) if they could review your PR.
Keep possible PR reviewers in the loop As a PR writer I want to make sure that others know about my work before I submit a PR.
Especially for larger and more complex pieces of work, it is best to keep others informed about the work that will go into the PR before it is raised. This assures the PR can be reviewed in a speedy manner and helps to address potential larger problems early.
Take time to consider every comment As a PR writer I will appreciate every comment and try my best to understand and address it.
It takes a lot of time reviewing PRs and comments are often an excellent learning opportunity. If you don’t agree with a comment made, try to find evidence to refute it. If you don’t understand a comment, seek out the reviewer or help from your team members to understand it. If you like a comment, leave a word of thanks.
Talk rather than type whenever possible
As a PR writer I will try to clarify feedback in person to help speed up the PR process.
Especially if there are disagreements between PR writer and reviewer, it is best to seek out each other in person and discuss in the PR. This also helps to reduce review times by being to clarify feedback immediately rather than
Expect the PR process to take a while As a PR writer I plan for the PR process to take a couple of days, especially for more complex features.
If we think of our work as being ‘done’ the moment we hit the submit PR button, we are bound to be disappointed. Waiting for PRs, understanding and addressing PR comments and especially doing the same multiple takes easily can take a couple of days even if we try to be as quick about the PR review process as possible.
Award all ties to your reviewer
As a PR writer I want to reduce friction from differing opinions as much as possible.
Some decisions about code are a matter of personal taste. If your reviewer thinks your 8-line function would be better as two 5-line functions, neither of you is objectively “right.” It’s a matter of opinion which version is better.
As a PR reviewer I will:
Review the PR as quickly as I can As a PR reviewer I want to have short review cycles.
We want to avoid having long PR cycles, for the reasons listed in the above section. Try to get onto them on the day they were raised if possible.
Approve only if I understand the intent of the PR As a PR reviewer I want to improve my understanding of the codebase.
You might end up having to fix the code you are reviewing in production. Don’t you want to try and understand it before it gets there? Approve it even if it’s not perfect (but it should always be better than the last time I looked at it)
As a PR reviewer I want to help continuous improvement. This is also part of wanting short PR cycles. No code is perfect - we want continuous improvement, not perfection! Be friendly in your comments, and favour suggestions and solutions over rejections (by this I mean “can we try and do this?” rather than “don’t do this”).
Think about these things too during the review process: Can we reuse any existing bits of code? Any common patterns that could be used? Any potential bugs that you can see?
Make sure I give positive feedback along with identifying areas for improvement As a PR reviewer I want to give fair and reasonable feedback.
PR reviews with negative comments are extremely demotivating. Give review comments in the same way you would like to receive them, and don’t be afraid of giving some praise!
Talk rather than type whenever possible As a PR reviewer I try to do in person conversations about the PR whenever possible.
When there are many comments on a PR, it is often quicker to go through all comments in one go in person rather than exchanging comments back and forth.
Use images and gifs wherever possible (preferably dogs) As a PR reviewer I try to bring joy to the PR writer whenever I can.
Receiving critical comments on a PR is not always the best part of our everyday work. If we can lighten the mood a bit, then we should!
Practical Guidelines
- When performing a code review, create a review with all comments and submit at the end, rather than submitting individual comments.
- Aim to provide at least one piece of good constructive feedback per review, the codebase will only improve if we can all hold each other to a higher standard
- If you’re not sure what kind of things to give feedback on, ask to pair review with another dev that does, and get them to talk through their thought process (maybe we can just make this a regular thing as well?)
- Distinguish between comments that you hope to address before merging, and non-blocking comments
- If you have comments you would like addressed, submit with no explicit approval or denial
- If comments are non-blocking - comment and submit with approval
- Smaller PRs. We should start trying to write our code in such a way that we don’t have to wait until the very end to open and merge a PR.
How To Review
Knowing what to look for while performing a code review is probably one of the hardest parts of being a good reviewer. This medium article has a particularly comprehensive guide to the kinds of things we can be thinking about while reviewing code (see section Performing code reviews).
FAQ
When can a pull request be merged even if there are issues? Large pull requests that introduce technical debt but no bugs to application code may be merged in if:
- Addressing the technical debt would require significant additional work and/or expand the scope of the card
- A card has been created detailing how the technical debt can be addressed (and brought to the product owner’s attention)
- There is little risks that the technical debt in the card may propagate (e.g. that the pattern is picked up in other areas)
What can I do to avoid large pull requests?
- Embrace the single responsibility principle: try to make changes about one thing and one thing only
- Submit partially completed work: Especially when using feature toggles, it should be easy to make pull requests even before a piece of work is completed
What do I do if a large pull request cannot be avoided? As mentioned above, large pull requests should be avoided whenever possible. However, if they cannot, a number of strategies can be employed to have them reviewed as efficiently as possible:
- Identify possible reviewers while writing the code and keep them in the loop as to what is being implemented.
- Spend a good amount of time on the description for the pull request and comments within the code to make the burdensome task as easy as possible for reviewers.
Useful Links
Writing PRs:
Writing and performing:
- https://medium.com/palantir/code-review-best-practices-19e02780015f
- http://blog.ploeh.dk/2015/01/15/10-tips-for-better-pull-requests/
- https://simpleprogrammer.com/5-things-learned-from-first-pull-request/
- https://hackernoon.com/the-art-of-pull-requests-6f0f099850f9
- https://mtlynch.io/code-review-love/?utm_source=programmingdigest&utm_medium=email&utm_campaign=397