Chris Zempel · 2023-06-27 · engineering, pull request, ruby, rails

12 Tools for Quality Pull Requests

As you stare into an empty text field, the blinking cursor invites you to engage in a crucial part of being a professional software engineer — writing a pull request. What will you write? Almost every idea you will come up with is downstream of the habits you've cultivated and the information you've consumed. Your attention to your upstream (or lack of) will define your practice — your customary, habitual way of doing something.

How is your practice of writing pull requests? On one hand, you will improve as you engage in shipping work as part of a team because you'll be repeating the act of writing and reading pull requests. On the other, occasionally applying conscious and deliberate attention to your practice will yield large and rapid improvements. This is when you work on your upstream — the habits, tools, and techniques you lean on when you start authoring a pull request. Often, this is harvesting your experience to see and think in relevant ways you couldn't before. Sometimes, though, the world sees fit to make it really easy on you.

My teammates recently put out some pull requests which were so nice they (metaphorically) shook me by the shoulders and shouted "you should also be doing this!" Before I can share with you exactly what they did, you must also understand a fuller picture. Sometimes beauty is the lack of something. Appreciating the lack of something is hard if you don't know it is supposed to be there. But when you understand the goals of writing a pull request, it then becomes possible to know which elements you can remove.

What is a quality pull request?

Quality means consistently meeting spec. For our pull requests, this means "do I have the right reviewers?", "do reviewers have all the information they need?", and "will someone looking at this in the future be able to access the 'what' and 'why' behind the 'how' of this implementation?"

Notice these are questions, not advice. Questions are resilient and adaptable to context. Advice is brittle. The best tools to keep in your upstream are the right questions.

Also notice this has nothing to do with "perfect." While it is tempting to focus on how you can write the perfect pull request, this is the wrong question. A focus on perfectionism is a great way to hide, hedge, and never ship. Or, perhaps more insidious, continually spend lots of wasteful effort when you could instead be learning how to achieve the same effect with less effort.

The goal is a durable, refined practice you can lean on to write a pull request that meets spec in the shortest amount of time.

Tools for writing pull requests

Here is a cheat sheet I use to write quality pull requests, quickly. And below I explain my thought process for each question so you can start using this in your own work.

#Cheat Sheet

On Writing

Do I have all the right reviewers?
Do reviewers have all the information they need?
Is my information organized in descending levels of scale?
Will someone looking at this in the future be able to access the "what" and "why" behind the "how" of this implementation?
Can I clarify my title for other contexts using prefixes, conventions, or other keywords?
Is there supplementary information I could add as footnotes?

Using Details Tags

You can hide nonessential information using details.

Example:

<details><summary>Toggle me!</summary>Peek a boo!</details>

Adding Footnotes

Here is a simple footnote[^1]. With some additional text after it.

[^1]: My reference.

On Structural Artifacts

Should I include some kind of diagrams, illustrations, or screenshots?

✅ substantially modifies code with a lot of branches of essential complexity
✅ includes a large amount of discrete before/after states
💡Integrate this element of your practice earlier into your development to minimize effort

On Responding

Am I coming at this from the right mental frame?
Does my feedback block approval?
Could this be phrased as a question or observation?
Is this a nit?
Does my review include something I like?
If you don't know what to say but you like it, just use ✨.

Going deeper

Do I have all the right reviewers?

Maybe security, or your platform team should look at this. Perhaps this change will impact another team's functionality, or there's a specific subject matter expert who will be able to tell you information you didn't even know you should think about.

Do reviewers have all the information they need?

This is figuring out what information you need to include. One big tell you should mention something is when you're surprised by a piece of information not otherwise indicated in the code. At the least, this information should be present in your pull request. Other information that's usually good to include? The strategies you used to implement, and maybe even the ones you tried and then threw away. Perhaps it is worth explaining the behavior of the system prior to your change.

Ultimately, how well you do this depends on your wisdom. Wisdom is the ability to correctly predict the future in a specific context. Pay attention to what questions you're asked and what information others seek. Over time, you will learn what you don't need to say. I've noticed that at Aha! we often don't need to include lots of detail on bug fix pull requests. There is the problem, a link to the bug tracker, and a fix contained in the pull request. Unless the fix is nonintuitive, this often adequately expresses all the information a reviewer needs to make an informed decision.

Is my information organized in descending levels of scale?

You've determined what information needs to be present. Now you must determine how to arrange it. This generally means some combination of:

Prior system behavior — useful in pull requests addressing bugs with nonobvious fixes
What the key abstractions are and why they were introduced — provides a mental anchor for your reviewers to understand and contextualize what code they are seeing
Little big details — is there system behavior, or other finer-scale details which have an impact on the large-scale design of the system? This is worth prioritizing and highlighting earlier on.

Will someone looking at this in the future be able to access the "what" and "why" behind the "how" of this implementation?

This presupposes you have a system in which you track your work. In each pull request, we link back to the record which caused the work. Many of us include the record's reference identifier in our commits, and we have an in-house CLI tool which will conveniently branch off wherever we're at and include the reference identifier in our branch name (and thus our default pull request name). Internally, we colloquially refer to these records as "features."

If you have never had this experience, let me tell you: it is amazing. Being able to look through the contents and history of the shaping feature allows you to understand what was intended by the code change and why the change was introduced. Knowing this has helped unstick me many many times because I am able to confidently distinguish what code we no longer need and what we must keep.

Instantly recalling context from people I've never met 5+ years ago is just cool. I've started using Aha! Develop for personal work and side projects for access to this form of extended personal/organization recall.

With these bases covered, let's look at ways you can represent finer scale details.

Can I represent this detail with a comment?

When there is detail which only makes sense when you're also considering some specific code, include the detail in the code. For example:

"The space that needs to be added to text is now added <other place>."

Or perhaps you've introduced a finer-scale abstraction or refactor that's only worth calling out the first time a reviewer will reach that line of code.

We do if information.fetched? && information.company_name? many times in the templates, so I'm adding a convenience method.

Can I hide nonessential information using `<details>` ?

If there are pieces of information in your pull request that aren't essential for the first time someone's reading it, you can hide the information inside a <summary> component. I often use this for quick scripts to set up the system in a certain state, or for deeper explanations of a given topic.

<details>
  <summary> How do `details` work? Simple. </summary>

  You have your summary, and your details.
</details>

How do `details` work? Simple.

You have your summary, and your details.

Is there supplementary information I could add as footnotes?

GitHub added support for markdown footnotes. This is a great feature that lets you include supplementary information which does not fit well inside the main text.

Quick reference:

Here is a simple footnote[^1]. With some additional text after it.

[^1]: My reference.

More advanced use, sourced from the linked GitHub post

Should I include some kind of diagram, illustration, or screenshot?

If your pull request substantially modifies code with a lot of branches of essential complexity or includes a large amount of discrete before/after states, consider building some sort of informational aid for your reviewers.

I've found myself reaching for whiteboards on multiple features recently. Whiteboards make it really easy to flowchart and enumerate the intended execution.

Even just in GitHub-flavored markdown alone, you can create some pretty incredible pull requests.

Diagram, illustration, or screenshot

Putting these together during development while the information is already in your head isn't a lot of extra effort for you. Doing so can produce a tremendous reduction in effort for your reviewers.

When revisiting older code, a visual cue can quickly remind the reader what this feature looks like, and trigger memories about related context they might have.

Can I clarify my title for other contexts?

Often your pull request's title will be the main piece of information that others see in systems outside where you keep your code.

When I have a feature that spans multiple repositories, I often update my titles to include the repo name in brackets up front so it's easy to keep straight.

PIZZA-2022 [pizza-design-system]: upgrade toppings to level Infinity and Beyond
PIZZA-2022 [pizza-app]: upgrade toppings to level Infinity and Beyond (PIZZA-2022 is the format of our Aha! feature reference numbers.)

Other pieces of information you might want to consider updating with:

Include important keywords to help with searching
Use a consistent convention such as feature ID prefixes
Name the "what" or "why" instead of the "how"

Tools for responding to pull requests

Am I coming at this from the right mental frame? It is easy to fall into the emotional trap of over-identifying with your work — causing you to beat yourself up as a failure over each change request. Or maybe you're setting your expectations to a place where change requests will make you feel resentful that you didn't receive approval instead of grateful someone is trying to make your work better.

Consider that someone else is spending their life and attention understanding your work and helping you move it forward. Often, these are extremely experienced and skilled peers. Think about a time before, when you were newer, when your sole desire was to learn and improve. What would you have done to get this kind of generous feedback? What would you have given? Then think about now. What's actually different? Nothing.

We're all human. Sometimes there's deadlines or other kinds of pressure. If you find yourself emotionally reactive, don't type anything. Step away. Take a moment and figure out why you're feeling the way you are. Chances are you're being too harsh on yourself. Be kinder to yourself, or reach out for support.

The goal is consistently shipping quality work with your team.

Could this be phrased as a question or observation?

This is a technique I picked up at Aha! which I then realized was pervasively used by effective open source contributors. Once I became cognizant of this technique, I now see it everywhere.

When you're not sure you have full context, or want to soften your feedback, phrase it as a question. A question might be:

Could we inline these styles instead of modifying our webpack config for just this one place?

Where an observation would be:

I noticed in my testing that in <some conditions> I'm still seeing <cookie that shouldn't be there>.

Phrasing your feedback in terms of questions or observations is humble and generous. It leaves space for what you might not know and keeps the focus on the work you're doing with your teammates. Not on who was doing it.

More examples:

Is the property relevant at all? Not sure what that distinguishes.

Can this be private, maybe others too?

The reason why questions are so effective is they create a conversational loop where you can make progress with someone else. This keeps momentum up and the focus on the work you're doing together. Addressing the question, implementing feedback, clarifying context, etc.

Such change suggestions can still be submitted with a go-for-launch approval if the code is still going to be improved in the near future. The author can choose whether to address it now, or potentially later.

Does my feedback block approval?

If you have feedback that requires change and you're certain it's necessary, use an active voice and explain your reasoning. This could be a simple bug, a small refactor, or a large need. Either way, if you have feedback that will block code approval, make this clear.

This is a moment when you can be especially generous and raise the skill level of your team. Here is an example of some particularly good feedback:

It's better to test the call one more layer down the stack instead of the private method, as the private method can be refactored now without potentially breaking the dependency on the Tracker layer below.

That tracker_stubbed config is defined here and just DRYs up the boilerplate.

This feedback explains "why" and "what," then provides guidance on "how." I tend to reserve this for situations where I have absolute certainty, the existing implementation is otherwise functional, and I can't easily phrase it as a question.

Is this a nit?

A nit is a small, non-blocking and potentially opinionated change suggestion. I find it is best to call these out directly as a nit:

nit: this method seems awfully similar to SomeClass#this_other_method. Could we use that instead?

nit: why not !private? instead of listing every other option?

Using language which directly implies this is a nit and maybe we want to do it (at the author's discretion):

Maybe a better way to access this: <code example> I think this will also give you the titleized version, removing the need to call toUpperCase below.

Or it is a personal preference:

I prefer to do <thing you're doing> like this: <code example> Because then you get x, y, and z.

Does my review include something I like?

It's easy to end up only commenting on things to be critical of. Take time to acknowledge the parts that are good, too.

When you see something you like, say it. If you don't know what to say but you like it, use ✨.

I really like how <thing is working>

Thanks for the well-reasoned approach here

Those capybara specs 💯

Nice. Bonus points for meme style.

Choose tools to support your purpose

Remember, the goal is a durable, refined practice you can lean on to write a pull request that meets spec in the shortest amount of time.

Questions are resilient and adaptable to context. Advice is brittle. The best tools to keep in your upstream are questions.

Here is the cheat sheet

Aha! is happy, healthy, and hiring. Join us!

This blog post only exists because of efforts of other engineers on the team. I stopped multiple times in the middle of reading a pull request with the recognition 'this is so well done.' These moments prompted me to dig in further to understand what specific techniques were causing this reaction in me. If you want to build lovable software with this talented and growing team, apply to an open role.