As the next round of the FOSS Outreach Program for Women (OPW) approaches, my mind turns to mentorship, and lessons learned when dealing with newcomers to open source projects. Many open source contributors have been in the FOSS community for long enough to forget how painful their first experience contributing to a project was. As the coordinator for the Linux kernel OPW internships, I get to help newcomers go through that experience every six months. I’ve learned a lot about how we, as open source reviewers, maintainers, and mentors, can help newcomers during their first contributions, and I’d like to share some of the perspective I’ve gained from OPW.
The Newcomer’s Perspective
As a newcomer, you’ll come at the project with enthusiasm and determination to do your best to make a really good first contribution. You’ll try to find all the documentation for the project you’re working on, and read through it, only to realize it’s completely outdated and incomplete. You’ll ping mentors and ask questions, but you may not be able to reach the right person to answer your question. So you do the best you can with the resources you find, cross your fingers, and submit your first contribution.
It’s common for newcomers to blame themselves when they make mistakes in their first contributions. You’ll cringe, wring your hands, smack your forehead, or maybe even put your head in your hands. Then you’ll sigh and try again. No matter how good the documentation for contributing to the project is, how meticulous you are, you will slip up at some point. It’s highly likely that your first contribution will be riddled with simple, easily fixable mistakes. And that’s fine, because you are going through a process of learning something new, and expanding your skills. The most productive contributors see each mistake they make as a growth opportunity, instead of a personal failure.
The Maintainer’s Perspective
As a long-standing open source contributor, you may get contributions from newcomers all the time. You’ll see several of them make the same mistakes over and over again, and if you have enough time, you’ll update your project documentation to help people avoid those mistakes. Often you don’t have time, and the documentation doesn’t get updated. Or you’ll think that something is so blindingly obvious that everyone should understand it, without realizing how much specialized experience you need to have that knowledge.
At some point in as a maintainer, you will be completely overloaded with contributions from both newcomers and familiar, trusted contributors. It’s easy to review those contributions from long-standing contributors, because they know your expectations and the rules around contributing. You trust them to write solid code containing very few bugs. So you review the contributions from trusted contributors, and put off reviewing contributions from newcomers until you have a long block of time to really thoroughly review the contribution.
It’s tempting to just go through the newcomer’s first contribution from start to finish, thoroughly commenting on all the things they missed. The maintainer’s mindset is, “Ok, I have time, I should share my knowledge with this person who is obviously missing some tribal knowledge they need to contribute to my project.” From the newcomer’s perspective, what they experience is their contribution being ignored for days or even weeks, followed by a very long email full of nit-picky comments on coding style, criticism of their code structure, and even comments about their spelling and grammar. Even if the review is fair and neutrally worded with a focus on their technical mistakes, it still feels very harsh.
We Can Do Better
How can we make this process better on both sides? How can we make the first patch review less harsh, and still respect the maintainer’s valuable time? Can we make turn around time on first patch review even shorter? When I was the xHCI driver maintainer, I started experimenting with a different way of reviewing newcomer patches that I think might help address all three of these issues.
The Three-Phase Contribution Review
Instead of putting off reviewing first-time contributions and thoroughly reviewing everything in the contribution at once, I propose a three-phase review process for maintainers:
- Is the idea behind the contribution sound?
- Is the contribution architected correctly?
- Is the contribution polished?
You can compare these contribution review phases to the phases of building a new house or taking on a remodeling project. The first phase is a simple yes or no on the architectural diagram, the big idea of the contribution. The second phase is getting all the structural issues correct and making sure the plumbing and electrical all connect properly. The third phase is making everything polished, sanding off the rough corners, and slapping on a coat of paint to match whatever color the bike shed is currently painted.
Phase One: Good or Bad Idea?
The first phase of the contribution review should only require a simple yes or no answer from the maintainer: “Is this contribution a good idea?” If the contribution isn’t useful or it’s a bad idea, it isn’t worth reviewing further. The best action in this case is to refocus the newcomer on a better idea or a completely different area they could work on. Or open a discussion with the newcomer and other contributors as to what should be done to address the issue in a different way.
If the contribution is worthwhile, but you don’t have time to go onto the second phase of patch review, do NOT say nothing. Instead, drop the contributor an email that says, “Thanks for this contribution! I like the concept of this patch, but I don’t have time to thoroughly review it right now. Ping me if I haven’t reviewed it in a week.” This builds the newcomer up by expressing appreciation for the time and effort they put into creating this contribution, and lets them know they’re on the right path. It also gives you incentive to actually move onto phase two, because the contributor will bug you again if you haven’t reviewed the contribution.
Phase Two: Is this Architecturally Sound?
In phase two, you review the contribution to see whether the code (and only the code) is architecturally correct. Focus on whether the code is sound at an architectural level. Is the code behavior correct? Are they modifying the right functions, or does the code need to be moved around? Have they structured their build files correctly? Do they need to refactor any code? Do they need to get buy-in on the code structure from other maintainers? Are there potential hazards or tricky parts of the code that the everyone needs to review carefully?
You will need to squash the nit-picky, perfectionist part of yourself that wants to comment on every single grammar mistake or code style issue. Instead, only include a sentence or two with a pointer to coding style documentation, or any tools they will need to run their contribution through. If their patch needs to be updated against a newer version of your project, or a different maintainer’s upstream repository, point that out. Avoid nit-picking every instance where they violate your project’s contribution style rules. Your eyeballs may be bleeding from the number of camel case variable names or variables names that use variable type encoding, but take a deep breath and ignore that. Let them explore the tools, documentation, and fix (most) of their mistakes on their own.
Double check and make sure the documentation and tools actually document the mistakes you see in the code, and if they don’t, update them. Your documentation and tools should clearly spell out the format of a valid contribution, and if they don’t, you need to address that technical documentation debt. If you don’t have time to address that technical documentation debt, tell the contributor what needs to be fixed, and see if they have the time to address it. Don’t be silent just because you don’t have time to fix it.
Phase Three: Is the Contribution Polished?
From a newcomer’s perspective, after phase two is complete, they’re hooked on getting their contribution in. You’ve worked with them on an architectural level, and they know you want to accept their contribution. They’re emotionally invested in getting their contribution into your project, and they’ve learned a lot by going through a couple contribution revisions. Thank the contributor for being patient this far and remind them that you’re willing to accept the contribution, but they need to clean up a few small things first.
Now is the time for phase three: the polishing phase. In this phase, you finally get to comment on the meta (non-code) parts of the contribution. Correct any spelling or grammar mistakes, suggest clearer wording for comments, and ask for any updated documentation for the code. It doesn’t make sense to create documentation for the code until the code is structurally sound, which is why the documentation phase comes last. You may also need to encourage them to write a better commit message, mark the patch to be back ported to stable versions of your software, or Cc the right maintainers.
As a newcomer, this third and final phase can be more painful than the architectural critiques in the second phase. Many young programmers lean towards science, math, and technology because they don’t feel like they excel in writing or people skills. Contributors may also be writing in a language that is not their native tongue. That’s why this nit-picky phase comes last, so that the contributors get over their embarrassment after they’re emotionally invested in getting their patches into your project. Be gentle, patient, and compassionate. As a maintainer, you may suggest comments or patch descriptions that you hope the contributor simply copy-pastes into their patch. You may have to just edit the patch description yourself.
How Does This Benefit Maintainers?
I’ve found that this three-phase contribution review process saves me (as a maintainer) a lot of mental stress. The first phase is a simple yes or no question (“Is this a good or bad idea?”), which means I don’t procrastinate on reviewing first time contributions. Being up front with contributors about not having time to review their contribution can initially feel like shirking duties, but I feel a mental load lifting when I get over that and simply say something like, “Hey, this patch looks like a good idea, but I don’t have time to review it right now. I’m heading to a conference next week, and need to work on my slides. Can you ping me in two weeks if I haven’t reviewed your code?”
If you’re honest with contributors about your time commitments, they know their contribution is wanted, and they can pass your time commitments onto their boss or program manager. Also, if you find yourself delaying contribution review often, that may be a sign you need a co-maintainer or you need to ask other contributors to do more code review.
The absolute worst thing you can do during phase one is be completely silent. The newcomer doesn’t know whether their contribution is a good or bad idea, and any discussion that needs to happen with other maintainers to modify the fundamental concept never happens. That’s why phase one is a simple yes or no answer, in order to get the code review ball rolling.
I’ve also heard some maintainers state that they want to dump all their review into phase two. They have precious little time, and they fear they will forget specific feedback if they break code review into several phases. I will often notice nit-picky coding style issues during my architectural review, and I will make a note to myself to nip that pattern in the bud in phase three. Keeping a dated text file per patchset or even replying to the patch but only adding your own email address in the To field will help you keep track of the issues that need to get addressed in phase three.
Often by the time you get past the architectural discussion in phase two, many of the nit-picky comments you initially wanted to make may have been addressed. A conscientious contributor will look at the documentation and tools you point out in phase two, and will address most of them in their next revisions. What will be left for the polishing phase three is mistakes made because of undocumented tribal knowledge, or rules that are undocumented because they differ from maintainer to maintainer within the project.
Try It Out!
The following three-phase contribution review process should help both maintainers and newcomers:
- Is the idea behind the contribution sound?
- Is the contribution architected correctly?
- Is the contribution polished?
Maintainers will be able to respond more quickly to contribution review if they focus on just answering one question during the first phase of review: “Is this a good or bad idea?” Newcomers will be encouraged by a timely email that states whether the basic concept of their patch is sound. Both the maintainer and the contributor benefit from splitting the actual code review into an architectural discussion, followed by a polishing phase. Maintainers will save themselves time if they simply point out documentation and tools contributors should use to make a valid contribution, and the nit-picky polishing phase is saved for after the newcomer is emotionally invested in getting their contribution into your project.
I think this process should both save maintainers time, and decrease the bounce rate for newcomers, so I encourage you to try it out!