Pull requests as a conversation starter

conversations

Conversation is not normally regarded as a way of measuring quality, but at MongoHQ we’ve realised that it is one of the most accessible and obvious ways of evaluating the quality of pull requests. Chris Winslett explains how that came to be and how it all started with a large pull request:

Our API team was looking to introduce a new version for the API. We’d talked about the new version long enough that we basically had the spec in mind driven by security, routing, and user experience changes. There were only a few changes in the interaction with backend systems, so the versioning change would be self-contained within the API.

The short run

With a well considered specification, the development was amenable to a sprint which is what happened. One person sprinted for about 2 days, and BAM! After the 2 day sprint, the developer had a pull request ready with 1,612 lines to be added and 724 lines to be removed. The Pull Request description was 255 words and contained 32 route changes. It was a good first go of the project. But there was trouble on the horizon.

The first indication that something was wrong was that no one commented on the pull request in the first 24 hours. Then, no one commented in the first 48 hours. It would be four days and much prodding of the team with no movement that the realisation came. We needed to have a heart to heart about the pull request because at this point it wasn’t going to be merged. The problem was the pull request’s size.

When we talked to the team three things came out. First, there was too much code in the request. This meant it required too much effort to keep all the changes in your head and that meant nobody thought they had time to look over the code. We added a bold text warning to the top of the pull request and set about devising a new plan.

The smaller steps

The new plan would break the problem down into much more manageable chunks but before that could happen the essential elements had to be extracted from the big pull request. That meant the mechanism for accessing the new version and the required security changes would be broken up into two blocks of work and then to test them one sample route would be selected to be pulled. The next step was logically grouping the various path changes into releasable portions. We would only start on the next group after the prior group was complete. Having multiple small pull requests open simultaneously would be as detrimental as having one large pull request.

With the breaking up of the pull request planned, the best person to do the work was the original developer as he had already given us a prototype and roadmap for the holistic change and the code already had tests which could be migrated over. It would have been easy to have just cut and pasted the code over into new pull requests but that wasn’t going to benefit the project. With two weeks away from the pull request, we let the original developer return to the code, less the assumptive understanding that they had acquired and ready to take on the task of breaking up their code. Now they could critically and surgically pull out the code, work on it and create the desired smaller pull requests. Each one would then be offered to the team one at a time.

The little changes

The code finally released was significantly different from the original pull request. Both the versioning scheme and security testing were improved due to the team being able to focus on their separate requests. The process went further too, with implementation choices being challenged and changed by the team. The developer noticed the change too. Rather than the silence that greeted the original pull request, the new smaller pull requests were seeing comments appear in minutes, triggering the conversations that we all need to help polish the code. Best of all, the team found the experience of working on the smaller pull requests better and much more in keeping with how they like to work too.

The lesson learnt

“Don’t sprint into a waterfall” is the lesson we took home from this. Since the big pull request incident, there’s been other code that has needed to be updated and with conversation-based development that process has involved bringing all the team in on reviewing code and ensuring there’s a deep resource of knowledge for the future. If we’d pushed ahead with that big pull request we could have potentially left some team members ostracized from the circle of knowledge. Where that initial pull request was a net change of 888 lines of code, the broken up pull requests never exceeded 115 lines of net change. That 115 line pull request was also deemed in feedback to be too large. Looking at that feedback, future development will see the quality of a pull request judged by the conversations that it creates – those that do not spark conversations will be considered harmful.

Written by Dj Walker-Morgan

Content Curator at Compose, Dj has been both a developer and writer since Apples came in ][ flavors and Commodores had Pets.