Give some love to your PR | Hacker News Reader

Give some love to your PR

florio.dev

19 pointselleflorio9 comments

"Keep your PRs as small as possible"

In my org we struggle with this because of two combating philosophies -- the first one is the obvious one, "Keep it short and sweet so it's easy to understand and review."

The other side is, "Please stop creating PRs and checking in dead code that is never executed until a promised future PR unlocks it."

My particular product is very complex (read: messy) and minor changes involve lots of work to maintain back-compat.

When we tried the "small PRs", we gained easier reviews... but ended up with so many changes that require a future PR to complete the work, and the future PR never comes for some reason or another. Another problem for reviewers is "Even with the PR description filled out, I don't see the value of this PR unless I see all of it together."

If you're reading that you probably scoff, "Just fix your process" or "Tackle your technical debt so your PRs are simpler!" You're not wrong, but if only it were something that could be done in a week and not years :)

You are alone with this problem. Finding the right size for a PR can be really hard. Too big, and no one really wants to review it because it is too time consuming. Too short, and it fails to show the bigger picture. We struggle with exactly the same thing, with a bias towards too large PRs.

I am not even sure I can always agree with myself here. I tend to favor small PRs myself, but I also really dislike PRs that are only part of the solution. PRs should not only be about critiquing the actual code lines, it should also evaluate and discuss the overall architecture, security aspects, performance, readability, tests and probably other things as well. This is difficult to achieve with small PRs.

The solution is never to "just fix your process", in my opinion. The correct process depends on the people involved, the culture in the team and organization, the importance of the service in question, among other things. Finding the correct process is a never ending task :)

Isn't the solution to that as simple as viewing a chain of PRs together, by diffing one got branch against another?

I have two recommendations:

Make 20% draft for big PRs. The issue with big PRs is that the overall design is already committed. It's hard to tell the person to redo everything according to a different design. So instead, have the author create a mock PR that outlines the desired changes, and have that being reviewed. This allows to have deeper conversations than if the code is aligned properly.

The second thing is; do synchronous reviews for big PRs. Actually take the time to have a 1:1 video call and walk through the code changes. You will be surprised how effective that is, and it helps bond the team together.

My usual strategy is to write PRs for the end-to-end functionality of a deliverable (meaning they can get quite large), but with many small commits that can be stepped through during the review process.

When I'm ready to submit the PR, I rewrite the commit history so that each commit is an independent and easily consumable chunk of code - many of them might be isolated additions of helper methods, etc. One commit may be a database migration, another the REST API to talk to the database, each one having full tests for that discrete chunk of code.

The result of this is a PR possibly with 30+ commits for a large feature where each commit could be independently merged to master that, when stepping through one-at-a-time, tells a logical story of how every commit combines to deliver the full feature. Each commit message also provides a detailed explanation of what I've intended to do and why.

This means that when I'm near delivery for a feature I have a lot of administration work to do, but I think that the small, discrete, and clearly explained commits provide great value when trying to reason about the codebase down the line.

All of this, although 30 commits is getting a bit extreme. If you can write 30 commits, each with its own tests, your PR could almost certainly be smaller while still delivering value (not dead code). But I regularly end up with 10 commits in a PR.

One major benefit is bisecting. I often don't find I've broken some automated test until I push my "finished" code for review, and incremental changes, each of which independently pass all tests, can be easily bisected to discover the problematic change.

I keep seeing this “keep it small” mantra everywhere. There is a good reason people do large pull requests instead of multiple dependent pull requests. The reason is that current UIs are atrocious. For example GitHub plain doesn't support them which makes it an absolute nightmare to review them (irrelevant changes[1]).

Additionally, syncing the different branches with Git is a pain, especially when you have several levels of dependencies.

[1] I'm well aware of the fact that you can set the parent branch to something else than master. But in that case, you'll have to merge the parent branch after the branches which depend on it. This completely defeats the purpose of splitting pull requests.

With regards to commits, I think it’s better to _not_ worry about making them nice. I think time is better spent making the PR description clear.

Make as many commits as you want, just “Squash and merge” at the end using your pull request body for the commit message.

Please no. Squash and merge means that when someone (maybe you) comes across a strange-looking line of code in 18 months and uses git blame to figure out why it was added, they'll be presented with a 1500-line wall of text with no context.

Commits are a tool. Learn to use them.

Reply on news.ycombinator.com