So, you’ve found a nice Open Source project that has added great value to your own work and you want to give back.
Before we move on, let me stress that this isn’t anything personal. This article doesn’t criticise anyone particular, and the ranty tone is just for your reading entertainment. I do not want to discourage you from contributing at all, neither to our own work (e.g. jOOQ, jOOλ, jOOX, etc.), nor to any other product. Open Source works also because of your enthusiasm.
Also, do not take this article as a discouragement of criticism. You can criticise software (like any other product) all you want. There is in fact an established measurement of such criticism, and we’ve done so excessively ourselves. But in this article, we’re assuming that you’ve already made a pull request, and why you did that wrong.
After all, be a sport. Take this article with humour and say to yourself…
The following “sins” are committed by so many people, I just had to write this down. Here they are:
The 10 Pull Request Commandments
1. Thou Shalt not reformat
OMG! Please! Never ever hit that sweet Ctrl-Shift-F (or whatever keys your IDE binds that command of doom to). There are only two exceptions to this rule, and that’s
- When there’s a auto-format commit-hook anyway, if someone with a lot of power and ego insists on such rules (and millions of other extremely useful code lint rules).
- If there’s an open issue in the bug tracker that reads “Re-format all teh codez”
But let’s face it. You’re not in any of the above situation.
We’re all adults and we all work with programming languages (which all suck), which is why we accept the fact that the code is poorly formatted.
2. Thou shalt absolutely not fix whitespace
In case you didn’t get #1, here’s a specific case of it: Whitespace. Yes, I know. This horrible OSS project has mixtures of "\t" and " ", " ", " " scattered all over the place. It suffers from severe indecisiveness with respect to "\n", "\r" (damn mac users), and "\r\n".
It suffers severely from:
if (a == b) {
if (c == d)
{
} else {
}
}It even has occurrences of
if ( a == b)
{
if (
c == d
) {
}
}So what? Swallow your pride. Don’t fix that. Does your pull request read “Implement a sophisticated algorithm to calculate the significance of the difference between the mean and median distance between stars in our universe” or does it read “Fix the darn formatting”?
The answer is in this interesting article here: http://blog.jooq.org/2014/07/25/top-10-very-very-very-important-topics-to-discuss/
3. Thou Shalt not refactor
I know that this series of if-else could better be represented by a strategy pattern, and that we should really finally apply the chain of responsibility pattern for a cleaner separation of behaviour, and hey, if we can add a little command pattern, we can even undo the whole thing, and that the world would be a better place if we removed all static (so incredibly evil) and final.
But you’re not maintaining that code, and your pull request reads “Fix Javadoc typo”. So, no. You do not try to inject your superior design skills into the project that you won’t maintain any bit right after that clever pull request of yours.
4. Thou Shalt not move
Oh, this one is the worst of all. Why even think of this? Why does path.to.MyClass have to be moved over to other.path.to.MyClass? Or worse, to other.path.to.BetterClassName? If you get this right (and you’re lucky enough to be relying on git’s black magic file moving discovery), then it might not matter that much, but what if you’re using SVN or TFS or some other version control system where moving is an explicit action? You’ll screw up all history, you’ll produce diffs (even with git) that are hard to review. You’ll just help all hell break loose. So don’t move stuff. Leave it there. It sucks, yes, but… No!
5. Thou Shalt not rename
And why would you want to do that? OK, the variable name was called thing and grammatically correct, it should really be called things, but in fact the word thing doesn’t yet fully describe what the thing really is, so you rename it to objects
Added value? Zero.
Added version control history? Tons.
Added regression risk? Some. (have you thought of reflection, backwards compatibility, serialisation, etc. etc. whatnot)
This is called noise and you shouldn’t do it.
6. Thou shalt document
This is an absolute no-brainer. I’ve written a whole post about that alone: http://blog.jooq.org/2013/02/26/the-golden-rules-of-code-documentation/
To be fair, this one is highly maintainer-specific. Please ask about how to do documentation, if in doubt. Usually, just mimick the level of documentation that is already present, e.g. any of these:
- None at all (what the hell)
- Only Javadoc / API doc
- API doc + lots of references to issues in the code
- The whole code repeated as prosa
- The whole code repeated as prosa in Shakespeare-English
7. Thou shalt not implement more than one thing in a single commit
This isn’t about whether you should have one or several commits per task. You should have only one.
This is about whether you should have several tasks in a single commit. You shouldn’t.
Imagine the code review process of the maintainer of the software you’re contributing to, when exposed to the following alternatives:
Good:
- Fixed Javadoc typo
- Fixed formatting (agreed with maintainer. I swear, I checked)
- Added tests (note <- test driven development! Tests go first!)
- Implemented tough complex algorithm
- Implemented new API
- Fixed typo
Bad:
- Heres teh codez (5 million files touched)
You get the point.
8. Thou shalt ask the vendor / community first
I know you really need this and that feature and you know exactly how to implement it, and are sooo ready to contribute, man this is exciting.
But here’s the ugly truth: No one has expected you, let alone waited for you and your contribution. Your contribution will cause a bit of work for the maintainer(s), who will have to maintain it for years down the line, while you’ll probably leave and never contribute again.
You can, of course, use a pull request as a documentation of what you plan to do, but if it’s not just a small typo or other trivial change, or something that’s on the roadmap anyway, just please ask first if you can do it, and if your implementation idea is along the lines of the project. You can save yourself and the maintainer a lot of unnecessary work.
9. Thou shalt not demand
If #8 above doesn’t resonate with you, here’s an additional point of view. You do not demand that your feature be implemented or that your pull request be merged. There are millions of reasons why things are done in one or another way in matured, popular OSS projects. You don’t understand them (yet), and you’ve based a lot of design decisions on your own local use-case. This use-case is very interesting and greatly appreciated, but it has to be planned and integrated thoroughly, not with the first implementation draft that came to mind.
So, if your pull request gets rejected (chances are high), don’t be angry. You might have just needed to consider #8 first.
10. Thou shalt accept the license terms
Here’s the thing. All Open Source projects / products out there are Open Source for a reason. Many of them (like jOOQ, for instance) are Open Source merely because Open Source allows the vendor to very easily distribute their software, getting a lot of marketing traction for the upselling offering. In the SaaS world and in many other worlds, this idea is called Freemium (someone actually went through the hassle of creating a website about “freemium”: http://www.freemium.org). Others want software to be free as in freedom. Again, others simply don’t care and use whatever license they find (often, ironically, the wrong one).
When you contribute, you will need to accept contributor license agreements, or rights and title transfer agreements. Some licenses consider their CLA to be implicit (e.g. read this article by Red Hat about why CLAs are unnecessary when contributing to Apache Licensed software). Others have more important reasons why they need a CLA or transfer agreement.
This is important to the maintainer of the software, and it should be important to you. If you care about free as in freedom (for instance), go back to #8 and ask the vendor / maintainer, if you can keep the copyright for your contribution, becoming a licensor of the software. If you do not follow #8 and provide your pull request before signing any agreements, well then just sign the thing, please. I mean, your ideology and dogma is important, but then again, not that important.
Conclusion
By now, you either had a laugh or you’re completely mad at me and this article. It wasn’t meant to be taken too seriously. But here’s the serious part to it. Open Source is everywhere and it is inevitable. GitHub gave Open Source an even bigger boost. Contributing is (technically) very easy. This has pros and cons.
Most often, contributions to projects that are hosted on GitHub are very welcome. But don’t take contributions for granted. Don’t take appreciation for your work for granted. Don’t take Open Source for granted. GitHub is only a hosting platform (before, we used to host on SourceForge). It is not an ideology. It doesn’t mean you can do anything you want with what’s hosted on GitHub.
TL;DR: Don’t just send a pull request. Engage with your vendor and with the vendor’s community. It will be a much more rewarding experience for everyone.
| Reference: | The 10 Things Everyone does Wrong when Committing Pull Requests from our JCG partner Lukas Eder at the JAVA, SQL, AND JOOQ blog. |
As an OSS maintainer, the biggest problem I’ve found is PRs which have not been tested, and/or have no tests. Or more problematically, have bugs. Usually both.