Pull Requests (what could be a good process)

Jan 5th, 2015 By Esther Saimpou

I hear people saying it and I say it my self very often lately… Nothing should be released without a Pull Request! The thing is that even I don’t raise them anymore And yes, I say that with shame…

— myself :)

I found myself constantly saying we should enforce them, but the truth is no one can force us to do anything. Thinking about it though made me realise that it is our responsibility to make sure we have a Code Quality Assurance, as we have a Product Quality Assurance. We wouldn’t add features or bug fixes in stable and release it without going through QA. So how and why do we accept going through without Code QA? (yes I am referring to a Code Review) :)

Apart from code quality though there’s another great benefit in Pull Requests. They are a great way of sharing knowledge in the team of what’s going on in our code base. I have found myself going through a merged and closed PR, just because I wanted to know what is the new feature and how is it achieved. Saying that, expanding our knowledge is another benefit from PRs.

I do expect some booing here but I’ll say it anyway. These are some of the benefits you can achieve with pair programming and perhaps with more freedom. I am not suggesting it’s equivalent or should replace pair programming! Just stating some of high level reasons that came to mind of why we want to do and we like Code Reviews.

Being a member till recently of a very big team of 6 FE developers and we had to have a good system. (Here I have to say we were very lucky our FE lead introduced them and had implemented a mergeable through Github Pull Request that made the process very easy and automated) We were all doing so much work in the exact same files and we were changing the codebase so fast and so often, that I don’t think we could have achieved so much without Pull Requests and Code Reviews. We prevented wrong methods to be used and we were able to maintain our code neat and and with minimal duplication.

The next question now is what are the reasons that even though we want to we do not actually do them? (At least not always).

Well my experience in the team was that the process yes it is great BUT it has it’s own challenges. So when the team was later divided in to smaller teams we just stopped doing them with a feeling that now we can sneak code in production without any criticism.

Some of the challenges to list:

• Pull Requests were raised and they were not reviewed and merged for days even weeks in some cases.
• The above resulting in the developer responsible for the branch to have to maintain the branch and going through the pain of conflicts again again.
• To lose his/her code at the end cause the main codebase has changed so much that the conflicts wire not making any sense anymore.
• Huge Pull Requests with hundreds lines of code to review that no one could actually be bothered to review.
• Code Reviews that were leading to an unresolvable argument of the approach resulting again in non merged branches that remained there for ages.

These are only a few reasons just on the top of my head that we do not like Pull Requests and Code Reviews. Now these reason’s can be solved perhaps by setting some rules. I will make a few suggestions and I would love to hear more, or better solutions.

• In our current way of workflow small branches should be opened for each ticket raised against an Epic branch that is to be released.
• The developer’s raising the PR should have the responsibility to create a small request easy for colleagues to check.
• Small single tickets in small single branches. That’s why we have small tickets to divide the workflow properly! Even tiny changes can go in as a separate branch (meaning a separate PR). Perhaps there could be an exclusion where in one PR there are a couple 4 max, tiny, like single line changes, each very clearly stated in the raised PR. On the other hand perhaps even that should be avoided, if it’s proven people don’t know where to draw a line.
• The developer’s raising the PR should have the responsibility to find a person to review his/her code. If no one prompts to do it, the developer could nominate someone and ask them to!
• Then the person nominated would have the responsibility to do the Code Review within the day or the latest early the next day if the PR is raised late in the afternoon.
• If the person nominated is extremely busy should still have the responsibility to find someone else to the Code Review instead.
• In the worst case if it can’t be done by no one soon, communicate, coordinate and come to an agreement for when it can be done. In any case do not leave it for more than a day or two, it’s supposed to be a small Pull Request in the first place.
• If the Pull request is too complex and has more that one features/bugfixes implemented, the person nominated could deny to do it, and the developer should cherry pick and divide it’s branch into two or more, as needed.

Perhaps at some point we should try finding a way to merge from a raised PR with a fast forward so that we can cherry pick easily, remove and leave for later each small ticket. Otherwise do a manual merge and achieve the same.

Originally published at www.gumtree.com on January 5, 2015.