Git + Github is an amazing combination. Git has been considered the de facto distributed source control system for a long time, its popularity being clear and undeniable. And GitHub’s about page states that it is “..the largest and most advanced development platform in the world“. At the time of writing GitHub notes that there are 56+ million developers contributing to 100+ million repositories on their platform. Impressive.

In spite of its prevalence, many of the companies that I’ve worked with, even those that I consider to be mature engineering companies, have an undocumented approach to authoring and reviewing Git pull requests. Twice recently I’ve recommended a common sense approach to my clients in effort to encourage them to discuss, agree and document their preferred practices.

Here are those recommendations, and though far from exhaustive, they provide a documented set of suggestions that, if nothing else, should stimulate discussion.

Authoring Pull Requests

• PRs and branch names should contain a story/issue number in order to aid review and provide context.
• PRs and branches should be short-lived.
• PRs should be as small and as easy to review as possible.
  • 10 files = small, 20 = medium, 50+ = too large
  • Small PRs tend to get reviewed quickly, merged quickly and deployed quickly.
  • Large PRs are unlikely to get reviewed quickly as your colleagues will be more reluctant to engage in time consuming distractions. Therefore you’ll receive slower feedback.
• Consider the reviewer as you work, be empathetic.
• Preview your PR and if it looks difficult to review, restructure it.
• DO NOT mix refactoring changes with feature changes, unless absolutely necessary.
  • It makes a PR harder to review due to mixed intent.
  • Refactoring can result in a ripple effect and can in turn lead to larger a PR.
  • You might need to revert either the refactoring or the feature changes independently. Reverting such distinct changes independently becomes impossible with mixed intent commits.
• Use draft pull requests to seek early feedback where appropriate.
  • Use this when you are not confident of the best approach.
  • Early feedback may fundamentally change your approach for the better and save you time.
  • Avoid polishing your code before getting such feedback. Critique is harder to absorb if you’ve polished your code.
• Try to be open minded and welcome critique. The goal is a frequent feedback loop, consistent code and high quality software.
• Limit WIP (work in progress). Don’t blindly add to a large pool of PRs. Actively review before adding to the pile, unless it’s a business critical PR.
• Take ownership of your PRs. It’s your responsibility to get feedback, refine your PRs and get the change approved.
• Don’t let your PRs stagnate. Rebase frequently to ensure your PRs remains up to date with the base branch.
• Don’t be egotistical. Be conscious that you’re working in a team environment and that there is often more than one way to solve a problem. Whilst you may love your solution, others may not. Equally, have an opinion and welcome debate.
• Be courteous. Receiving PR feedback is an important aspect of continuous improvement and being courteous will encourage people to give PR feedback in the future. Feedback affords you an opportunity to improve the code you’ve written and your skills.

Reviewing Pull Requests

• Make yourself available to review other peoples PRs. If you don’t, other people won’t offer their time to review your PRs.
• Offer suggestions in your PR reviews, not just commentary.
• Try to give positive feedback as well as recommendations. This helps to highlight good coding practices and helps to foster stronger team relationships. People will also be more receptive to recommendations if you acknowledge great code too.
• Be prepared to pair on the problem rather than just throwing commentary over the wall and running away. People may not understand or be able to implement your suggestion, so be on hand to help. This can also help prevent PRs stagnating and help with change throughput.
• Following a review, approve a PR, but don’t merge it. The merge should ideally be done by the PRs author, since they will likely be closer to the story/task, appreciate release dependencies and are better placed to release the code change (this is of course subject to your CI/CD pipeline).
• PR comments are no substitution for conversation. If the GitHub PR comments are going backwards and forwards, it’s a good indication that you need to talk in person rather than adding to existing PR dialogue.
• Look for divergence from code conventions, patterns, over engineering and consider consistency. We all spend a lot more time reading and maintaining code than we do writing it. Thus consistency and simplicity is very important. A code base that looks as if it has been written by one person is much easier to maintain and extend than as if written by ten people.
• As per authoring PRs, be courteous. Being courteous doesn’t prevent you being direct, candid or opinionated but the author of the PR is more likely to absorb your feedback if you are courteous.

I don’t believe these practices to be contentious. I do believe however that they help to drive up a teams cohesion as well helping teams to optimise change throughput.

It was actually a tweet from @chris_herd that prompted me to share these recommendations. Covid will no doubt change how and where product/engineering teams work. We’ve embarked on a journey that’s likely to see an increase in remote working, thus documenting such agreed team practices is more important than ever.

✍️ Written over spoken: documentation is the unspoken superpower of remote teams. The most successful team members remotely will be great writers

Companies are searching for ways to do this more effectively. Tools that enable others to write better will explode

— Chris Herd (@chris_herd) February 18, 2021