Thoughts for Pull Requests Authors
Here's some thoughts on how to structure PRs and how to communicate your intent of code change clearly.
Pull Requests, merge requests or any mechanism to introduce code to the main production/development branch is usually guarded by one or more code reviewers that approve, request changes and provide feedback before allowing merging of code. Will refer to them as PRs. We merge code in order to ship what we worked on from the code branch. Now this article will cover things to consider as an author of PRs to ensure a smooth response from reviewers. We can’t guarantee that but having a great PR is certainly easier to review and something developers enjoy to review. PRs typically have a title, summary and a diff. The summary is where we add the majority of the pre-requisite information about the diff which is context, how did you implement it, why did you implement it this way, test plan and deployment plan if any. The diff is the actual code change, it can be commented on. I should preface this by saying that those are my thoughts so don’t take them as rules as they also can evolve or be challenged. Will start with the summary first. Every reviewer should know what this PR is about, i.e. there should be a context of what this change is, what it relates to and how it fits into the bigger picture. For example, if we are building a health check api endpoint then the context should be something like: ``` This change aims to introduce the skeleton code required for the health check endpoint part of ensuring we have a way to query for our api health in one api call. ``` This context outlines why we are doing this code change. Keep it as brief as possible unless it is required to be lengthy. The reason for keeping it brief is that the reviewer can either be from your team or a different team. If they are from your team then they would have enough context in their head. If they are not on your team then most likely a longer context may lose the reviewer’s attention and cause them to skip the summary all together because they have other things on their plate as well. In the case the reviewer is interested to know more, I encourage you to add a reference to where they can learn more. It could be a ticket, project roadmap and etc… Now that was the context part. Include images or screenshots if you think it is worth it, otherwise keep it simple. I encourage the context part to be the first part of the summary. Next part in my opinion is to list down the changes in a list format unless you’re expected to write it in a paragraph. The list items should link back to the context and be on a high level but packed with information regarding the change. Ideally, the list should be in order of the changes appearing in the diff but changes maybe all over the place so organise it as you think makes logical sense. A good list of changes may look like: ``` - Added the endpoint definition returning a mock status as it is going to be implemented in subsequent PRs - Added an integration test suite to call the new endpoint ensuring we get the expected response for the happy case ``` The idea is to keep the list small and reflect the changes in the diff without reiterating what the details in the diff do. We now helped the reviewer understand the changes on a high level and how we relate them to the context. This now will allow the reviewer to start thinking on how to better improve the code and ask questions that are related to the logic of the code because they have a high level idea of what to expect coming up in the diff. Next up, the notes section. This is where you can note down reasons on why you went with your current implementation and list down potential alternatives with reasons on why you didn’t adopt those alternatives. It is also where you can influence the reviewer to a certain way of thinking about the code. I don’t think there is a best way to go about this. I believe this section is important for the reviewer if you think it is going to help out the reviewer understand something that is assumed or hidden otherwise don’t include any notes. It is worthwhile to answer any questions you expect reviewers will block your PR on in this section as it communicates to the reviewers that you have thought about the problem thoroughly. Obviously briefer notes are better but don’t lack on quality here because this is where your personal touch in the PR lies. By personal touch I mean this is where you showcase your train of thought to the reviewer and give context into why things were done a certain way. Next up is probably the most important part for accountability. The test plan is where you note down how you tested the code and proved it does what it is supposed to do. Now I assume you the reader to not be working on a codebase that has a state of art automated testing setup that verifies everything for you with minimal manual testing and everything gets tested automatically. You probably want to QA manually or there is a QA person that does this for you. Either way, I encourage you to test the code you write because of the fast feedback loop you gain when you fix it right away if it is broken unlike handing it to QA, make sure to note the steps you took in your test plan and also explain the rationale for why you took them. Apart from writing automated tests, having a good and well thought out test plan is always something that boosts confidence in the reviewer to be less skeptical of whether the code actually works. I recommend you always show the before and after state on modifications of existing code as a benchmark. If the code is just covered by automated testing and you’re confident about that then don’t stress. It is always better to reduce the size of the summary if you think that’s the case. At the end of the day it is your PR. Please also conform to any work policies here as they may help you when accountability is needed to be shown. That was a lot of words. Hang on, there is just one more part that I think should be included as well. A deployment plan is a section I believe with adding if merging the PR requires unusual steps to be done right after. Things like merging after a script was run, merging multiple dependent PRs, whether should be merged on a specific order, day and so on. I won’t comment much here because I think this is an area where it is done differently at different companies. If there is an unusual deployment plan then I think it is worth also writing a revert deployment plan. This is useful for what to do if the code goes wrong very soon after deploying. It is better than panicking when the code breaks in production and whipping a list of things to do right away, it may invite a chance of doing things wrong. Reverting is not always the best way to fix a problem but maybe the quickest way if there is no external side effects of the code. Of course, this was a list of things that I think should be included in every PR where it makes sense. Context, notes and deployment plan can be safely ignored if the PR is quite simple in logic but doesn’t mean to ignore them all the time. Context helps so much when you expect the reviewer to be inquisitive into why you made the code change in the first place, to avoid potential back and forth in comments, I recommend including it. The notes and deployment plan sections I believe are not as much useful for every reviewer, the reason I say that is because usually you will have reviewers who are just reviewing the code for the sake of reviewing and not interested in digging deeper and improving the codebase as a whole. Nothing is wrong with that, it comes down to how much a reviewer wants to allocate of their time reviewing code and whether the code will affect them in the short term. If your new PRs don’t have a template showing up as soon as you create the PR then I highly highly recommend adding such a template. All major Git hosting services (GitHub, Gitlab and Bitbucket) support this. It not only allows you to describe a codebase wide PR template, it also allows those PRs that have lots of changes without much summary the chance to be communicated better. Now we’re done from the summary part of the PR which is usually harder than solving the code problem. Just kidding. Next is the diff part which is the difference in code from your PR's branch to the target branch. Usually red means removal of code and green addition of code. I always recommend reviewing your code before publishing a PR and make sure to give it a proper review. Update any parts you think are not good enough, note down the change in the summary if not already noted in the summary and make sure the entire code change is actually required, more on this next. More often than not, we come up with huge diff PRs. Absolutely huge. They regularly send reviewers away and are hard to keep in the head what’s going on. Not only that, usually the PR could be doing 5 changes that are not really connected to the main aim of the PR in the first place. This is where self reviewing the PR is an absolute benefit, question whether the change is atomic and must be added in one PR. If so, just note this down in the notes section of the summary and list the rationale, don’t expect the reviewer to just assume because you said it needs to be done in one PR that they will allow that, add supporting reasons. Multiple changes can be broken down into separate PRs and that’s the best way to do it. It also means you are aware of the changes you are introducing and want to keep the review process smooth which will make reviewers appreciate that even though they may not review your code right away. Ideally, while writing the code before submitting the draft PR, be conscious of whether everything added is required or there is extra fluff. If there is extra fluff then I recommend thinking twice about including it unless it has an immediate business impact. Sweet, now we fixed the multiple changes in 1 PR issue. It is time to think whether you can break down the code even further. Only apply this if the atomic change is huge. It is also a time to consider whether you could’ve solved the problem in an easier and less code consuming way. Of course don’t just rewrite it for the sake of reducing the size, sometimes it makes sense to do if the code looks bad in your opinion or it is not efficient at all. I believe this is the code author’s call. What I can say is to believe in your code and don’t think because it looks bad that it is not worthy of a submitted PR, more often than not the things that don’t look good are namings and not the logical flow. Thus invest in good naming of things and ensure you are consistent with names. I don’t think huge PRs should be taboo if they are atomic and well documented (having good summary and the code speaks for itself with proper naming). As you write more and more PRs you get the intuition of whether to create one branch or many before writing any code. It is not always accurate but make sure to utilise it. Of course, project timelines maybe more urgent than delivering a well documented PR and if this is the case, please prioritise that over quality as it may affect the business directly especially in smaller companies. Those are the thoughts I had for this article. At the end of the day, take it as mere opinion and use what you like if any. PRs are written by people and reviewed by people so be considerate of the reviewer and ensure the PR is not offensive. Cool. In summary, ensure to have a great summary even if it takes more time to write than just submitting the PR. It communicates well thought out solutions to problems and care to the reviewer. The diff should just be one change where possible and ensure it fixes the problem stated in the context of the summary.