Over the years I have contributed to various FOSS projects (both non-commercial and those owned by corporations) as well as proprietary ones. I’ve also played a role of a reviewer in all of those settings. Based on what I’ve seen and done I would like to propose some recommendations on how both sides can do better.
This is not a guide for how to conduct a review, you can find those elsewhere. This post is more about a strategy of accepting and offering changes.
The suggestions should be the most valuable for FOSS project developers who get or make their first contributions. Projects run by corporations can also improve their review process significantly as they gave me the greatest amount of pain in the ass even when I was payed to deal with them. Proprietary projects are the least affected by this due to the lack of contributors from the outside, but improvements in this area can help new developers on a project and maybe improve life of existing ones.
Why rules?
The process of offering and accepting changes is formal enough to be governed by rules. Involved parties already assume some roles and prescribed actions, so we might as well define a bit more specifics to make the process smoother for everyone by reducing uncertainties on both sides.
Sections below either cover approach to managing contributions/reviews or give more specific situational recommendations.
Mind contributor’s level of commitment
If someone is offering you changes for the first few times, gauge how much time a contributor is willing to invest and what interest does he have. Whether it’s done for the purposes of education, gaining experience or to satisfy a personal need matters.
The reason to figure this out is that some of the contributors aren’t even programmers and asking them to write code in a certain way might be too much as the person might not be aware of common programmer’s practices.
If a contributor is willing to spend more time, that’s great. If not, be ready to finish up the patch yourself. The fact that someone took the time to try and implement a change is a good indication that it will be useful.
Mind that fiddling with the changes long enough might bore anyone, so offer to do the remainder of the work if you notice that a contributor got stuck.
Don’t set the bar too high
Most contributions to most FOSS projects are one off. Some projects have lists consisting of tenth of bullets for a proper contribution that apply even to the most trivial changes! Those are usually popular projects, but I’m not sure it justifies the complexity.
Use a simplified process for trivial and irregular contributions if possible. And more generally think about finishing up changes yourself. The primary area where you might want to do this is updating tests, documentation, changelog and other supplementary materials which are really about project maintenance rather than functionality and are thus your headache.
In many cases explaining how you want the code to be is longer then making it look that way. So by adjusting the changes yourself you’re likely saving everyone’s time.
A large projects could have a dedicated queue of patches that need to be finished to streamline applying changes. However, this works even for a one-man projects as it can prevent piling up of patches and diverging them from the code base.
By the way, some large projects have issues with development precisely because they ask so much that the patches are easier to abandon rather than upstream. Sometimes changes are rebased and resurrected later on (maybe multiple times), just so that they can be abandoned too…
Don’t make contributors do your backlog
If doing a change in an ideal way requires significant updates in other parts of the project, accept the change and do/plan the updates. Don’t blackmail a contributor by refusing to accept the changes unless he does indirectly-related changes to the codebase. It is annoying to be told to make the code generally better when all you wanted was to extend/improve it a bit.
Contributions done as part of a job might be an exception as corporate-run FOSS projects are in part developed like this. However, maintainers should know how to do such changes better than a stray contributor who happened to touch that code.
More generally don’t make contributors write the code the way you would want it to be written unless it’s dictated by functional requirements. Otherwise you force a contributor to play a mind reading game with you, which is no fun. You know how it should look? Great, make it look that way or accept the changes without adding extra requirements.
Be realistic
Understand that contributors won’t have the same knowledge of the code that you have. A patch with a single-line change might have taken hours of experimenting and looking for a place and way to change the code. Also don’t forget that being presented with a solution might give a false sense that you’d find it in no time, while in practice it would take some effort.
Amount of time already spent on a patch might surpass initial expectations of a contributor who then just decided to not drop it halfway. Be happy that someone has done part of the work for you and continue from there.
Track submissions
Here’s what happens in large projects: you post a change, it’s reviewed,
approved (maybe even by multiple maintainers) and then not submitted… It’s
just left hanging there! I guess sometimes people think that the author can
merge into master and other times reviewers want to give more time for anyone
else to take a look. Either way, this is frustrating, especially when this
happens to trivial changes which should be merged right away.
Would be great if approved patches were organized into a separate queue for people with merge access to go through. I’ve never seen this, but it’s probably already implemented somewhere.
Save nit picking until the end
Calling out typos and stylistic issues is the easiest thing to do. However, it’s also the last thing to do. It’s similar to proofreading. You proofread essentially the final version, not a draft which will see a lot of changes.
There are, however, exceptions. Things that are hard to spot or look like unintentional changes can be called right away. Same goes for things that appear multiple times as that can indicate that the author just doesn’t consider it to be a mistake.
Overall, it would be better if both sides would first sort out important issues in the proposed changes and then switch to attending to minor details.
PR/issue templates
Either don’t use templates or use the simplest possible ones. The issue with templates is that they tend to grow to be overly generic with most fields not being applicable to any specific issue. This leads to long reports with little useful information and high level of redundancy.
It is puzzling when you want to report a bug or send a simple change and have to fill out a huge form. A better approach is to gather minimal amount of information at first and then ask for more details or give list of appropriate requirements later on. Don’t try to get a maximally complete bug report at first try, any such attempt is bound to fail anyway.
If you’ve left some review
Do not disappear. People will reply and you should read their replies and possibly take some action. This is especially important if only the author of a review comment is allowed to declare it as resolved. If you get lots of review notifications and it’s hard to sort them out, make a list of reviews you’ve actually participated in and check on them from time to time.
Don’t write multiple paragraphs if it’s not necessary. While in some cases giving all the details are vital, that’s not always the case and you might just waste your time and make it harder for others to assess state of the review. The problem is that you don’t know beforehand what case you’re dealing with. The best course of action is to leave a reasonably comprehensible comment and be ready to reply relatively quickly with additional details if necessary.
Don’t leave unintelligible comments. Seriously, extremely short comments with terrible grammar that you have to re-read over and over again in a futile attempt to make sense out of them are the worst. If you feel like something must be pointed out, but you don’t have the time, hint concisely at what it is and why it’s important. That might be enough or you’ll be asked for clarification later. If you still can’t do that, just don’t review or maybe ask to wait for your proper review for some time.
Avoid starting an unrelated discussion, because it’s likely to block or delay a submission. By doing this you’re effectively hijacking the submission for your own purposes. Don’t do that. Better create an issue or separate submission and just link to it. Maybe not right away, but once you realize the discussion isn’t directly related to what’s being reviewed.
If you want to contribute
Don’t invest too much time before consulting upstream. Otherwise, be ready to do changes in order to get things upstreamed. This doesn’t really apply to simple changes, but larger ones are better to be decided upon with maintainers to avoid producing incomplete or inappropriate implementation. If you’re fine with not upstreaming your changes, this is less important.
Look for contributing/code-style instructions. You don’t have to read them
in full right away, but at least skim for important stuff. Some projects
require Signed-off-by: in commits, some accept patches only through their
mailing list, etc.
See if there are tests. They might be easy to write for your case. And you should always try to run all tests with and without your changes to see whether there are any regressions.
If you’ve asked for a review
Don’t disappear. Yes, this works both ways. If you’re not really interested in upstreaming and just didn’t want to keep the changes to yourself, state that right away. If you’ve lost the interest in the process, explicitly abandon the submission. Otherwise people will review, ask questions and wait for you to come back.
Clarify review comments. If something isn’t clear, it might be better to ask for more information than to make assumptions which will later turn out to be false. It’s hard to gauge when this is appropriate, but asking yourself whether your interpretation of the comment and the way to address it are unambiguous before spending time on implementation is a good idea.
Tickets as contributions
While bug reports aren’t contributing code, they do communicate information about its operation. Similarly, feature request contribute to functional requirements.
This makes various rules from above apply to tickets as well. The most important ones would be: don’t disappear and don’t leave unintelligible comments.
Pinging
People forget to get back even if they are trying to remember. This is why if you want something to be addressed, you need to draw attention to it. This is usually done by posting a comment once in a while, in worse situations you might need to use some messenger or subscribe to a mailing list. The message can be as simple as “ping” or it can be a rebase in case of a code submission. The minimal amount of time in between pings is often a week, so that it’s not too annoying.
Caveats
For various reasons, things don’t go the way you wish, so transgressions are always possible. Everyone’s time is limited, people live in different time zones, unexpected shit happens, priorities and interests change.
Also keep in mind that just like doing a change can take much more effort than might seem from looking at the result, reviewing that result is trivial only for trivial changes. In general review is much more than just reading the changes, which is why complex changes require an effort to review and then re-review after they have been adjusted.
That’s it for now
I think that above sums up most of the issues I’ve encountered during review process and, hopefully, suggest reasonable ways to reduce their negative effects on the process.