Tips to successfully build your open-source projects #8. Don’t merge pull requests blindly
The most rewarding feeling you can get when building an open-source project is when others are contributing to it! It is truly exciting and it always makes me happy, even tiny code changes!
But be wary! A rookie mistake is to accept all incoming pull requests as-is. It's understandable: someone spent his own time improving your project, so you might feel pressured to merge his work in the hope that this will attract more contributions.
When you receive new pull requests, don't merge everything blindly, and make sure to enforce your coding standards to avoid your project becoming a mess.
Tests
This is the most straightforward part: does the change work without breaking anything else?
This is where having unit tests for your existing features becomes handy. And you should definitely expect this contribution to be tested as thoroughly as the rest of the codebase. Otherwise, how can you tell if the change is doing what you expect it to do?
Documentation
Is the code change appropriately documented?
- Pull Request Description: Did the contributor provided enough context about his code change for you to properly review it? What is the motivation behind the change? Why has it been done this way and not another way? What are the implications of the code change? Is it a breaking change? Even if this is just a bugfix: what is the bug, how do you reproduce it and how did the contributor make sure his change is fixing it? As you can see, a pull request without proper description is probably a non-starter.
- Generated Reference and General Documentation: As we discussed in Tips to successfully build your open-source projects #4. Document your project, your project should be documented and may have different forms of documentation. Typically, code comments in the codebase used to generate a full reference, and general documentation like a wiki. Does any part of your documentation need to be updated following this change? If there is one thing as bad as no documentation, it's outdated documentation: make sure to keep it up-to-date.
Code quality
Is the code meeting your quality expectations?
Someone is contributing to your project, that's great! But the code might not meet your coding standard: some functions might be too long or have too many parameters, the variable names might not be appropriate, the code logic could be simplified, or the overall code structure does not satisfy you.
There are many reasons for which a proposed code change does not meet your quality standard, but you may not feel comfortable appearing as a picky code owner by asking the contributor to rework his code even though it works.
This does not mean you should merge spaghetti code. Instead, don't be afraid to ask for adjustments and guide the contributor by telling him how to change the code to satisfy your expectations. If you are not willing to tell the contributor, or if you want to merge the change ASAP, then you can simply improve the given change by yourself.
Code consistency
This is different from code quality: a code submission can meet your quality expectations (short functions, clear code logic, appropriate code structure, …) but it may not have the exact same coding style as yours (inconsistent indentation, different number of line breaks, curly-braces syntax, …).
A single contribution may not affect your codebase consistency by much, and you may be inclined to let it go: you may not even see it. However, as more people contribute to your project, each with their own coding style, your codebase will slowly lose its uniformity.
Thus, you should not let anyone submit something with a different coding style than the one you defined for your library, or you will end up with a weird codebase mixing different coding styles.
One easy way to maintain this consistency without having to scrutinize every code submission and be picky about any code syntax issue is to make use of an automatic code formatter like clang-format. It probably will not exactly reflect how you are coding, but you can tune it, and it will help you ensure a sane consistent codebase without putting much effort.
The Greater Good
The contributor made a code change that solved one of his needs. But is it relevant for the other users of your project? And if it is, was the code change implemented with one of the best possible approaches?
For example, the contributor may have added a feature, but it decreased the overall performance of the project: what do you value the most in this case? Similarly, the code change may be valuable but was implemented such that it is causing a breaking change for others.
When reviewing a code change, you should think about its implications on your project as a whole: if it needs improvements or if it does not have its place in your project, don’t merge it or re-work it. Merging everything blindly, including irrelevant stuff or poorly designed changes can only lead to degrading your project quality.
Conclusion
It is great and exciting to receive pull requests!
But you need to spend the appropriate time to review, test, and possibly re-work them. This is a real art to do such a thing while keeping the contributor engaged, but that’s important if you don’t want to end up with a weird project that does everything and nothing at the same time.
I personally believe a project benefits from having a maintainer with a clear and opinionated vision of where the project should go. Of course, that does not mean the maintainer should be closed-minded and not open to any feedback or discussion. But it means that, in the end, there is one person making the decision about how it should be done and whether it is relevant or not.
Yes, it is open-source, but you are the project owner: you get to define the rules for your own project!
Read all my tips to successfully build your open-source projects:
Originally published at https://cylix.github.io on September 9th, 2017.
