With some pull requests, I've encountered the "probably should have emailed the list" and the "here's the style guide" in a single pull request.
As a maintainer, however, I'm far more lenient in these things. If someone takes the time to fix a bug or add a neat new feature in something I maintain, I'm greatful for it, and I'm not going to get anal about the pull request. If there's something about it I find needs changing, I'll accept the pull request and fix it myself (with a few exceptions), and inform the contributor of the changes I've made, why I made them.
I'd rather not make pull requests be an exercise in frustration for the contributor[1]. Usually, you want more people to help with your project, and frustrating your contributors isn't a good way to encourage that.
[1] I say this as someone who, with one pull request, spent several hours over a few days trying to get a 15-line change accepted (about 10 of which were comment lines), with lots of debate over the most of the requested changes being anal things like "add a space after the comma, remove a space after the =". I was damn near the end of my rope and ready to just say "fuck it, if you don't want my fix, don't take it, I'll just maintain my own fork."
You think that's bad, I had a 19 email back and forth with a developer over a patch I created to add three standard libraries to the linker options. He did not disagree with me that according to the specification, he was using functions which required linking to the aforementioned libraries. He did not dispute that adding these libraries if they were not strictly necessary would cause no disruption in the cases where it already built. He simply kept saying "well, it works for me and other people," and refused to compile simple test programs on his machine to see why my computer and the spec differed from his computer.
Months later, he silently incorporates the change...Talk about annoying..
I made my first contribution to a GH project a couple of weeks ago. It was only a few lines, and the maintainer had to make a few modifications before he merged it, but I got some really positive feedback from him and you could tell how grateful they were.
I've submitted pull requests to projects on Github and while there has sometimes been discussion, so far my changes have been pulled into the main project without issues.
Most of my commits are to homebrew for software I'd like in in brew but that isn't there, and also modifications to current software that is there to get it to compile on newer versions of Mac OS X.
Giving up ownership of code is hard. In fact, it's much harder than the decision to release the source code in the first place. The ability to accept a pull requests comes requires a willingness to accept another programmer's style. This means syntax, but it also means what code blocks get factored into functions, what is a class versus a function, what is a closure versus a member variable, and any number of decisions from significant down to trite.
A major maintainer of a project is generally are aware of the weak spots in the project and has a mental picture of how he would resolve them. This picture likely differs from what the pull requestor pictured, and so the implementation differs from what the maintainer imagined it would look like.
All of these small differences make the code feel foreign to the maintainer, and gives a feeling of losing control over the code. Overcoming this mental barrier is essential to being able to effectively collaborate on Github, and it's a skill that needs to be developed.
I love getting pull requests, and even when there's a problem with the patch, I usually just merge it anyways and fix the problems myself.
I do this because that's what people did for me when I was new. I accidentally crapped all over this SVN repo we were working on, and everyone was super professional about it - fixed all the things I'd broken, taught me how to use the tools I was using, and generally welcomed me into the group even though I had no clue what I was doing.
Posted to Rachel's blog, but repeated here for the wider audience:
Yes, I maintain several GitHub repos. Almost all of them are, in a sense, actively open to pull requests.
My take?
Random pull requests are fine, especially for minor tweaks and small bug fixes. Even better, if the pull request fixes a known, open bug from our Bugzilla instance. In that case, it would be good to mention the bug #. We don't have a formal style guide, but unless the code is just hideous, I probably wouldn't reject a pull request on stylistic grounds alone.
OTOH, if you come along and implement a major feature with no interaction with the existing team, things might get a little more complicated. In those case, it probably is better to establish some dialogue first, just to make sure everything fits. Likewise for fixing a major bug with lots of interactions / dependencies throughout the code base.. assuming your patch really does fix it, then yes, we'd take the pull request. But it would be good to talk through it first, walk through possible "unintended consequences," etc.
And, of course, emailing or establishing contact first is pretty much never a bad thing. Well, as long as one follows through with what they say they're going to do.
> And, of course, emailing or establishing contact first is
> pretty much never a bad thing. Well, as long as one follows
> through with what they say they're going to do.
There's the implicit engineering problem right there! You've always do a bit of implementation/thinkening before even going that far. Don't want to be the Flakey Sir Promise-A-Lot, do you? But of course it is sometimes hard to draw the line if it's a reasonably interesting problem ... and that's where hearts get broken and egos get bruised.
If there is something on your todo-list for one of your open source projects, add it to the issue tracker. That way contributors can see it's on the road map and have a place to discuss it.
Even better, for you and others, write down everything you can think of about the feature and what obstacles you are likely to stumble upon when implementing it.
I think this is great, it also lets your repo followers know that your not abandoning the project but are actively thinking about it even if you dont have time to work on it at the moment.
My complaint about Pull Requests is that they shift responsibility for doing the grudge-work from the submitter to me. As someone receiving a contribution, I have to read the contribution, make sure it merges cleanly, make sure it passes tests, etc. I'd rather say "Looks good to me" and then let the requester do all that work. (For simple cases, there is a button to automate the merge, but that doesn't run the tests.)
This, incidentally, is how code reviews work at Google. The reviewer (and owner, if you are submitting code to someone else's project) only check that the change looks good. The submitter is 100% responsible for ensuring that the change is good.
Once the reviewer says the change looks good, then the submitter gets permission in the source control system to actually submit the change. That's the feature I want from Github. I want to say "looks good, please merge" and then let them have access for one commit. (I believe gerrit works this way.) Then, the next tool I'd like is hosted unit tests and results on Github, so that the submitter can immediately see that they merged their change correctly and that the project is in a good state.
Oh well. It's probably time to go back to hosting my own repositories.
This is closer to how we work at internally at GitHub. Everyone files pull requests for non trivial changes, and are responsible for merging and deploying them safely. Edit: This works because we all have push access of course.
This changes when you consider open source projects. Any change that is simple enough that I can scan and give my thumbs up, is likely one I can scan and click the merge button. Immediate Travis-CI results tell me everything is happy.
Anything where the merge button isn't sufficient is probably not something I'd like a stranger to push to my repository. If they're not a stranger, I'd seriously think about sharing collaboration access.
But hey, we're not necessarily done with Pull Requests. I have various ideas like this I'd like to explore at some point.
I've never maintained a hugely popular open source project so maybe my opinion on this will change when that happens, but at the moment I am always over the moon (very happy) when someone submits any Pull Request to one of my projects.
And hey, sometimes I comment & close the request with "Thanks for this patch, but this isn't something I want to support." And that's OK!
My point is that my message to the author of this comic is to be bold and keep sending patches.
hard to say without seeing the pull requests, but if you edit compiled code and call it a typo you're probably at risk of doing a lot of things wrong.
perhaps you just need to spend a little more time learning about a project before trying to push your changes. seems like all of the issues presented in the comic could have been avoided by paying more careful attention.
That's definitely a change I'd bring up with the project first. I agree that having compiled code in there is bad, but it's typically not something a first time pull requester touches. Same goes for project files (Gemfiles, package.json, licenses, changelogs, whatever).
Every time I have tried to jump into a project, it has required a couple hours to figure out the compilation method, unit test runner, peculiar inheritance structure, or messy class names. This is simple stuff that the author left out of the Readme because he wasn’t expecting contributions. So I woudn’t blame the contributor if she didn’t fully understand it before the pull request.
sorry, what distinction are you drawing between "generated" and "compiled"? even "combined", for all intents and purposes, doesn't discredit the point:
if she was editing the wrong files, the answer is to do a little more research before diving in.
Just an example from my experience; if you're using lesscss and you're committing the generated css files to source control, someone cloning your repo and trying to fix a UI bug would probably just jump into the css file and make the change. They may not know that those files are generated by the .less files.
if she was editing the wrong files, the answer is to do a little more research before diving in.
That's not always as easy as it sounds if a codebase is large, uses non-standard build environments, violates best practices (like unnecessarily checking in generated files, and, if it's really necessary to do so, not making obvious that it's a generated file -- assuming it wasn't a simple oversight on Rachel's part), ...
In my opinion, pull requests should be submitted even if you're not sure they're exactly right as long as you've done a reasonable effort to make it so:
After all, familiarizing yourself with a codebase is done by working with it, and asking for help when you're stuck -- and that's exactly what a pull request is: a request for comments, where you show others what you've done so far. Of course you could have used the mailing list instead, but if there's this nice site available where you can publish your code, which keeps track of changes and provides a commenting and message system, why not make use of it?
I hate to reject code technically works, but comes with technical debt (may break in another compiler/OS, doesn't follow naming/coding style, lacks tests/documentation, etc.) I can't nag to much "fix this, and that, and that, and change name of that", but OTOH if I don't then I'm the one left with the technical debt.
On few occasions I've spent more time on integrating someone else's changes than it would take me to write it myself.
Still, of course everybody learns and has to start somewhere, so I welcome pull requests.
I maintain a few open source repos on github, including the rails jquery-ujs adapter and several other projects [1], and I've seen the exact responses and behaviors she describes, sometimes from people on my own teams. And it burns me up every time I see it. So much so that I wrote an article on the subject [2]. Quoting from my own article (yeah, I know):
> More importantly, to open-source authors: realize that the worst thing we can do is discourage people from submitting patches or starting and engaging in conversations about our projects.
I personally LOVE when people submit pull requests to my projects. I'm not always able to pull them in, particularly if they take the project in an undesirable direction. But I always try. Style doesn't match my "style guides"? I'll either ask you to fix the formatting, or I'll just pull your patch in and fix the formatting myself (it usually takes two seconds [3]). Documentation contains a couple inaccuracies or worded weirdly? I'll pull it in and change what needs to be changed. No tests? Ok, this one is a bit harder, but I'll either ask you to write a test, or I'll pull it in when I get time to write a corresponding test. Still, some code is better than none!
Seriously, it irritates me to no end when maintainers discourage people from contributing. The only thing that irritates me more is when people act entitled to my time to help them fix some implementation of this free software, but that's another story. People who submit pull requests of any kind almost never fall into that category.
Now, I do prefer people to open an issue before submitting a pull request, but that's only because I feel bad if they spend time coding something that is either going in the wrong direction or has already been fixed. But if you're willing to take that risk, by all means, pull away!
Bumping into friction and problems when contributing to open source projects is hardly limited to github.
A long while ago after hearing all about the virtues of open source software i decided to try and contribute to a project. I found something which needed implementing and decided to have a little go at implementing it. After a bit of discussion on the mailing list, i came up with a fairly minimal implementation which was a good starting point at least, and posted a ticket to the tracker.
For about 7 months nobody seemed to take much notice of the ticket, when all of a sudden i got a message: someone else had independently worked a new implementation, and only just noticed the ticket. In effect 99% of the work i put in was ultimately wasted.
Still was a good learning experience. Nowadays i think of open source projects as being more about people in a community rather than being merely about contributing code.
Does not only happen with GitHub and pull requests, but in any open source project, I guess.
I've recently submitted a patch to the mercurial-dev list (as requested). The answer was positive but asked for tests and a better commit message.
Few days later, when I had time to fulfill the request, I found out that one of the main developers fixed the issue in a different way, with an unrelated commit message and with no tests.
Most probably he did not even notice my patch attempt on the mailing list. I guess this happens all the time in every OS project. I still found this discouraging.
Another issue that wasn't covered is duplication of effort.
Imagine working on a feature for weeks, only to realize that other people with more experience and knowledge of the project are doing the same work.
What's the incentive from the maintainer's point of view to reduce duplication of effort? The maintainer's not paying either developers, and they have a backup if one of them doesn't come thru.
But from a developer's perspective, it's frustrating to put efforts into a patch that doesn't end up being used.
> What's the incentive from the maintainer's point of view to reduce duplication of effort?
The maintainer has a lot of work that needs doing, thus s/he wants to avoid duplication to get that work done. Also, s/he will know how disappointing it is to write software that is not used, and s/he will want to keep programmers happy.
I've ran into situations where my pull requests duplicated effort made by older unmerged pull requests. I know I should've reviewed all the existing pull requests before coding, but repos with more pull requests make that workflow pretty tedious. When you view a file on Github, it shows you a list of contributors to that file. I'd like to see Github also add a feature to show a list of pending pull requests affecting that file. This helps contributors avoid duplication of work as well as providing users an easy way to identify potential bug fixes that are pending review.
Unfortunately, just because you do hard work doesn't necessarily mean the world will need that work.
Look on the bright side and consider it practice. You may not have actually contributed to those projects in the end, but surely you learned something.
As a maintainer, however, I'm far more lenient in these things. If someone takes the time to fix a bug or add a neat new feature in something I maintain, I'm greatful for it, and I'm not going to get anal about the pull request. If there's something about it I find needs changing, I'll accept the pull request and fix it myself (with a few exceptions), and inform the contributor of the changes I've made, why I made them.
I'd rather not make pull requests be an exercise in frustration for the contributor[1]. Usually, you want more people to help with your project, and frustrating your contributors isn't a good way to encourage that.
[1] I say this as someone who, with one pull request, spent several hours over a few days trying to get a 15-line change accepted (about 10 of which were comment lines), with lots of debate over the most of the requested changes being anal things like "add a space after the comma, remove a space after the =". I was damn near the end of my rope and ready to just say "fuck it, if you don't want my fix, don't take it, I'll just maintain my own fork."