On shipping smaller PRs

Introduction

In my last I talked about the importance of shipping smaller PRs. In this article I’ll give some practical examples for how PRs can be split into smaller chunks. The examples aren’t organized into specific themes. Rather they’re arbitrary examples that demonstrate various dimensions for splitting PRs. Small PRs are great. They’re faster, easier to review and safer than their larger counterparts. We’re not talking about trivially small PRs. We’re talking about PRs that can be logically split into sensible dimensions. Not the one liners. So none of this advice applies if your PR is already small. Don’t arbitrarily break up a PR for doing so… that’s not the principle here at all. Without further ado… let’s dig in and examine a few examples.

Refactoring

Let’s start with a pretty common use case. Let’s say you need to change a method’s name and update the broken tests. Sounds simple, right?

Let’s look at the required changes. Firstly, we’ve got the core step… we need to move a method from one class to a different class in a different file. Then, we need to update all the tests for said method. Oh, and we need to update all the callers too. They need to call the new method. However, it’s usually never that simple. So let’s increase the authenticity of this example, by adding some expected complexity.

Let’s assume some incompatibility that’ll need to be fixed. In my example, I was forced to change how we were mocking functionality in the test. I also had to change some instance variables in my test. Also, I discovered some bugs and gaps along the way that needed to be rectified. All fair game when you’re making a change to a large codebase.

So of course, we could do this all in one go — make one big PR with the refactor, test fixes, caller updates, mocking changes and bug fixes. One big downside is the time to ship. It’ll take a while for other folks to review it. Also, it’s not always clear what’s changing from what’s not. It’s not always clear what moved from where to where. The other random changes blend it as insignificant and create a poor historical record for others to question later.

What to do instead?

How could we break this up instead? Think about your end state and work backwards. Then strategically bring some of those changes forward.

• Mocking changes — I created a PR that implemented the mocking strategy for the tests that would be impacted by my refactor. This is a small PR that can be quickly reviewed and creates an awesome historical trail for others trying to understand the change later. If anyone later wonders why I’d ever change the mocking strategy in a test, well… now there’s a PR a commit message dedicated to that explanation.

• Move the tests without refactoring them — another neat strategy is moving the tests to their new home without making any changes. That makes for a seamless review. “Hey, I’m just moving this test from file A to file B”. It’s so much more intentional and clearer. Instead of being buried in a sea of other changes, folks can now see I just moved some code around without edits.

• Bug fixes — these can also get their own dedicated, small PRs. I had an instance where a function was improperly swallowing an exception. This would have broke after the refactor, so I fixed it first.

• The original change — with the smaller PRs outlined above, your original change becomes smaller and easier to review. With the tests already moved, and all bug fixes and related refactors out of the way, you end up with a much slimmer PR which can be reviewed carefully, and quickly.

Separate implementation and use

Another good way to separate PRs is along the dimension of implementation and usage. We can introduce some new functionality with some tests, and then add code later that invokes said functionality.

As a concrete example, let’s say we’re adding a new repository call that creates subscription records in your database. By repository, I mean an abstraction over your persistence layer. Your business logic would interact with the repository layer instead of talking directly to your database ORM, or whatever framework you have.

Again, you could just ship all of this in one PR. You could add your new repository method, any tests needed for the repository. You could also include calls to your repository method, and tests for those calls. Oh, and if you need any additional abstractions, like say, factories for test data creation, you could add those too.

What to do instead?

Alternatively, you could split things up into smaller, faster chunks.

• New dependencies — any new abstractions that your code will require can get their own dedicated PR. If you need to add some new factories for test data creation, or some mocks, or extend your test framework, a small PR is the way to go!

• New component and tests — you can create a PR that only contains your new repository method and its tests.

• Create a PR that calls the new repository method with unit tests (optionally include integration tests as well, depending on whether the code path is invoked in production)

Disparate use cases

Another excellent dimension to split PRs by is use case. If you have two different use cases, split them into separate PRs. Those should ideally be separate tickets where sensible too. Let’s say you’re adding repository calls to create and update subscriptions. This will require the repository methods, tests for each and possibility some additional testing abstractions (e.g. factories).

And remember… it’s almost never that simple. Let’s also imagine that you also need a few utilities to support your new repository method. Or, perhaps you need to refactor a few things your new method will touch, or fix some bugs.

Yes, you could just stuff everything into a single PR, but you’re tired of hearing me sound like a broken record now.

What to do instead?

Firstly, you want to create a dividing line for each use case. Consider the bullet points below being preceded by a for each construct that iterates over each use case.

• Random tidbits — All the random discoveries you made (e.g. refactors, bug fixes) can get their own small PRs. Those PRs will be pretty easy to review and you’ll get critical, focused feedback.

• Dependencies — again, you can dedicate small PRs to any dependencies you’ll need. These could be test abstractions (e.g. factories) or utilities your new method will use.

• Repository methods — finally, you can create a PR that focuses on the change you’re introducing. This PR would have one use case method (e.g. create subscription or update subscription). You should include unit tests, and optionally integration and end to end tests.

Subdividing method responsibilities

This one overlaps in principle with the prior example, but the similarity isn’t that obvious, so I left it in. The previous example tackled splitting out class functionality (e.g. create/update subscription). Here, we’re pushing the boundaries to remind ourselves that methods aren’t indivisible! We can also split method functionality into separate PRs if that’s practical.

Note that this isn’t always possible. In particular, this example works well when you have unused code pathways in production.

Let’s say you’ve got a create_subscription call that doesn’t do anything yet. Let’s say it needs to call a repository to create the subscription and it also needs to tell Kafka about the subscription creation.

So you could… and you shouldn’t stuff all of this into one PR.

• Add new call to create the subscription

• Add unit/integration tests for subscription creation invocation

• Add new call to publish Kafka messages

• Add unit/integration tests for publishing these messages

• Add supporting factories and mocks for creating boilerplate test data and mocking interactions with the repository and Kafka

What to do instead?

Instead you could, or should… partition our code starting in order of growing dependencies. Start with the items that have the least dependencies and work your way up the dependency tree. Again, consider the bullet points below being preceded with a for each construct that iterates over both use cases (repository call and Kafka publishing).

• Dependencies — Add the factories that will be needed by the code and mocks. Anything that’s required to support your code can get its own small PR.

• The use case — here you’ll be implementing the use case in its own PR. For Kafka, that’d be adding new calls to Kafka and the supporting tests, package integration tests, potentially in a separate PR. For the subscription creation, we’re adding repository calls to create the subscription with supporting unit and integration tests.

With this sequence you can get PRs out and reviewed quickly, keeping the team velocity up, keeping quality high and shipping safe code!

Conclusion

There we have it! A few concrete examples of different ways you can slice and dice your PRs. Smaller PRs are generally faster, safer and simpler for everyone involved. Today we looked at some examples in refactoring, separation of implementation/use, separation of use cases, component extension. I’d encourage trying at least one of these strategies and deliberately seeking feedback to see how well it benefits your team!

I’d love to hear any success stories, or if you have thoughts on the topic!

Thanks for stopping by.