I maintain many open source projects on GitHub and elsewhere (over 160 as of this writing). I have merged and/or closed thousands of Pull Requests (PRs) and patches in the past few years, and would like to summarize here many of the reasons I don't merge many PRs.
A few of my projects have co-maintainers, but most are just me. The bus factor is low, but I offset that by granting very open licenses and encouraging forks. I also devote a set amount of time (averaging 5-10 hours/week) to my OSS project maintenance, and have a personal budget of around $1,000/year to devote to infrastructure to support my projects (that's more than most for-profit companies who use my projects devote to OSS, sadly).
I don't like closing a PR without merging, because a PR means someone liked my project enough to contribute back. But sometimes it's necessary. I'm not trying to be a jerk (and I usually start by thanking the contributor to try to soften the blow of a closed PR), I'm just ensuring the continued health of the project. Below are the principles behind how I maintain my projects, and hopefully by reading through them you'll understand more about why I choose to close PRs instead of merging.
Principles for Evaluating Pull Requests
For the projects I maintain (and indeed, for most software I work on in general), I hold the following principles to be the most important, and will usually close a PR if it doesn't hold up to these principles:
Everything should be well-tested with automation
Almost every project I maintain has at least full coverage of the 'happy path' running through Travis CI, Jenkins, or another CI system. If You Breaka My Tests I Breaka You Face. With rare exceptions, I will not merge a PR if all tests aren't passing. I also will not merge PRs that have large amounts of new, untested functionality. I don't require 100% unit test coverage, but all happy paths should be tested.
Maintainabilty > completeness
I don't cater to everyone. I usually cater to myself. And for 98% of my OSS projects, I'm actually using them, live, in production (often for dozens or hundreds of projects). So I'm generally happy with them as they are. I will not add something that increases my maintenance burden unless it's very compelling functionality or an obvious bugfix. I can't maintain a system I don't fully understand, so I like keeping things lighter and cutting off edge cases rather than adding technical debt I don't have time to pay off.
Cater to the 80% use case
I see a lot of PRs for one-off functionality that I've personally never seen in the wild. Sure there are unicorn systems out there that need to configure hairy details in some obscure app... but I'm not going to include code for that in my projects because (a) I don't use it, so I can't vouch for it, and (b) it adds maintenance overhead—even if it's a 'simple' addition. If you are one of the unicorns, please fork my work. I won't get offended! My public projects are almost always meant to solve the most common problems; and I try to make it easy to go deeper by either forking my work or extending it.
Use proper syntax
Often, I have automated syntax review built into my automated testing. But when I don't, please make sure basic things like spacing, variable naming conventions, line endings, spaces instead of tabs, and the like follow the general style of the project. I will often merge code then fix the style myself, but it's much nicer to not have to do this, and I'm more willing to merge a PR that has no style quirks.
Don't change architecture
(Unless first discussed in an issue.)
I've had PRs where the entire project architecture or test architecture was swapped out. I will never merge something like this unless it's been thoroughly discussed (and approved) in an issue first. There's usually a reason (many reasons, in fact) why things are the way they are. I'm not saying my architecture or test rigs are always right, but I will not merge in sweeping changes to things which make it harder for me to understand how my own projects work.
Don't change more than 50 lines of code in one PR
(Unless you have very good reason.)
Too often I get a notification that someone submitted a PR, I jump over to review it, and I see 20 files changed with 800 lines of code added across 12 commits. If this is an architectural change previously discussed in an issue, I can understand a large PR like this, and I'll take the time to read through it. But anything more than ~50 lines of changes and my brain doesn't have the capacity to do a good code review in less than an hour.
Conclusion: 'No' is the default
One of the greatest ironies about this process is that some of the people who open issues and subsequent PRs and are the most [persistent|annoying|difficult] are those who want one problem solved in their own project but will vanish immediately after the code is merged. They realize (usually not explicitly) that if they can convince me to maintain their snowflake code, they can save themselves that ongoing technical burden.
If contributors are willing to establish a long-term relationship with the project, I'm willing to concede some control over the architecture. But they have to prove that I can trust them. A few of the best contributors to projects I started are ones who use the projects in their for-profit work, but donate an hour or two a week to help tend the issue queues, close out invalid issues, and submit PRs for simple bug fixes (especially related to parts of the project they're most familiar with).
To anyone else maintaining OSS projects: make sure you have a well-established set of principles by which you can evaluate PRs. And feel free to say 'no' when a PR doesn't meet your standards. So many projects get derailed by accepting too many new features without evaluating them for long-term maintainability, and it's a problem that's avoided by a simple two-letter word.
Comments
I see a lot of "I" in here. I don't see the need. I don't want to maintain. I don't this, I do that..
I think you're entirely missing the point and rationalizing it away for yourself because you like the project how you like it, regardless of what the community thinks.
He shouldn't have to cater to a community unless he wants to. If a community wants to fork and maintain its own version, a community can. Maintaining software can be a lot of work.
If you disagree, fork the code and do a better job. It hurts to have someone do that, but the brief pain of separation and failure is better than a growing resentment over working for nothing. Kudos to Jeff for being so generous with his time.
Well they *are* his projects, and *he* is responsible for maintaining them. This is not a democracy, and he doesn't have to maintain all kinds of features he doesn't want because others want him to.
If people want to change things radically they should just fork, and take the responsibility upon themselves to maintain it.
Is the maintainer of a project not allowed to manage it how they want? If Jeff is making this stuff because he uses it, then it's entirely reasonable for him to be driven by personal concerns, rather than those of any community.
@dick, there are also a lot of "I" in "Maintainabilty"…
one too few, even.
160 is obviously too much for anyone to do it thoughtfully. So obviously ended up with a long list why you do it blindly: I don't have time, I don't care, I don't this, I don't that, close, close, close. Leave 150 of them and do the rest 10: thoughtfully.
I guess I wasn't clear in the blog post itself—I do actually read through each PR, and at least skim the code changes. And it's fairly rare I'll outright close a PR before offering feedback and leaving it open to changes. Even in the instances I do that, I'll add a few paragraphs explaining why I don't think it's a great fit for the project.
I would like to see links to a few examples of PRs you have closed.
It would probably be pretty trivial for you to find those yourself. It is a little odd to hoist this burden on the author.
You're welcome: https://github.com/geerlingguy
Thanks, Jeff. Not many OSS gate keepers go to this length to explain their process and standardization. Way to set an example!
You seem like a really nice person. You honestly don't owe any of us your time or explanations, and whilst I'm sure you know that. I'd like to take this opportunity to say thank you anyway and apologise for the hostility from others. Following yours and others examples, I've begun trying to do a lot more for open-source, I hope to do more, which occasionally means killing things I've worked hard on. I've got some musings / thoughts below, but I'm not saying "do this"; just this is how I feel and my experience, maybe it helps.
From someone that submits PR's; I generally don't care "big-picture" if they get merged. I Know that probably sounds lazy, but I do feel like a lot of work needs to be done to educate people on both sides that it's not a repo maintainer job to merge their contribution; but it is the user responsibility to contribute to repo's they use where they can and do have code. It's also not cricket to say to someone that has never written a test that doesn't come from that background (maybe experience of writing that sort of test) that they need to write tests. What can be done instead is an issue referencing the PR can be raised and then others can borrow from that "prior art" to maybe just write tests or re-factor because that benefits the community. I'm not saying the people that do that should be the repo maintainer / original creator or team. What the people (I admit I've been guilty of this too) can forget is that it's EVERYONE responsibility and privilege to benefit the repo.
Getting merged for me is more of an instant gratification. If something is merged then great for the project those features are a part of, because it's more likely those will be cared for. It's also better for clients as I have no right to withhold paid for open-source work and if I die (godforbid); don't want to work with them or move on, they can keep calm and carry on. Maybe this is why some people push 800 changes is they just got paid. I know it's been the case for me in the past, and so whilst I'd advise they get paid more often and smaller amounts to facilitate repo maintainers needs; but it is a bit cheeky, not always practical! As long as I keep my repo up and have submitted a PR; it shouldn't hurt me or the maintainer or client.
It's all about mind-share at the end of the day; the more people's brains you can plant a thought germ in, the healthier the repo should be. Apologies if any of this was seen as telling you what to do (it was mean't to put across an honest non-advice perspective, maybe grow mind-share on PR's and etiquette).
Again. Thank you!
That all sounds fair enough to me. As a contributor, I would ask for two things of maintainers in general:
- Please include all of your requirements upfront in a CONTRIBUTING file so I can avoid sinking time into a pull request that would be rejected. Also, this file is an indicator that you do welcome good pull requests.
- Please respond to my pull request. Even if you don't have time to review it, please comment saying you don't have time and that it will have to wait. I feel like most of my pull requests are just ignored and I don't know why.
I agree with this sentiment. Having a CONTRIBUTING file up front can save a lot of headaches and maintainer time.
'spaces instead of tabs' (in 'Use proper syntax' section) link is pointing to the same article.
Sorry about that! The link's been fixed: https://www.youtube.com/watch?v=SsoOG6ZeyUI
I agree to reserve the right to say no. It's great that you share your guidelines. But IMO, the idea of rejecting by default and
your track record of >50% rejectionshould be an indicator that you might have to rethink how you maintain, how you document, and ultimately how your architecture the software that the majority of your contributors apparently misunderstand the direction and scope of.Or maybe you write JavaScript projects and your community is full of people who are still learning to program, if that's the case, I rest mine :)
Otherwise..
If this rather large amount of people's hard, honest, well-meaning community work is rejected by you, stop for a while and think about the amount of effort you've wasted by somehow giving a community the wrong expectation?
If it bothers you that people write functionality that you don't need in your own deployments, you can avoid the PRs and the resulting forking sadness*) by making your projects more extendible. Depending on your programming language, frameworks etc., you can make classes overridable.. even create plugin APIs.
Far from everyone will get the point. Far from everyone will thoroughly read the contribution guidelines. But it's worth a shot, a least that way you can always refer to the guidelines rather than explaining things in individual PRs.
Good riddance and thanks for your hard work!
*) Forks are usually a sad thing because they also fork communities and efforts to maintain common platforms.
Wait... what? Where are you pulling that statistic? I don't have the stat in front of me, but based on recent memory, I think the percentage would be way lower than that. Also, there are probably 5-10% of closed PRs on my projects (and elsewhere) that were "oops I clicked the wrong button in GitHub" PRs.
Only three or four of the projects have Node.js code in them; but in the case of JS projects (or other projects more approachable to beginners), I actually make more effort to merge beginners' code, because it's a way of getting them into the FOSS mindset, and I remember when I, too, was a 'green' dev and submitted my first patches.
For sure; almost everything I do is 100% modular and extensible, otherwise I wouldn't want to use it. I also try when possible to make all dependencies (if any) suggested rather than required so other users don't have a terribly-annoying dependency tree to work with.
Some good points for sure, though—but please don't suggest that by defaulting to 'No' a maintainer is doing a disservice to the community. I find it's far easier to prevent burnout by doing this... which means the community will keep its BDFL for a bit longer :)
Please use your admin rights to the comments to edit mine. I wrongly stated your track record as ">50%", it was a brain fart. Stand to be corrected!!
I've crossed it out with the venerable
<s>tag. Thanks! :)