I like to receive pull requests, but I like some more than others. Here are a few hints on how to make me love your totally awesome pull request and merge it instantly.
With a bit more than a year as an open source maintainer I’ve received a few pull requests. Some have been good, some have had room for improvements and some have required extensive work by myself to clean up before merging.
An Awesome Pull Request
To write an awesome pull request is not that hard, but requires a bit of planning and reading up on tools.
- Add an issue describing the feature before coding. It’s an opportunity to get feedback both on the suggested feature and on how to make it fit with the existing design/architecture.
- Learn git. As a minimum you need to know how to fork the main repository, create feature branches and how to rebase them on a refreshed master pulled from the main repository. Check out the contribution guidelines from Nancy for details.
- Limit each pull request to a single issue/feature. It’s much easier to review and discuss multiple small focused PRs than one large complex.
- So you have Resharper and found a bunch of unused usings and other things that should be cleaned up? You’re welcome to submit those non-functional changes in a separate pull request.
- Run any existing tests and make sure you didn’t break anything.
- Make sure your code meets the test coverage expected and coding standard in the project.
- Respect the architecture of the existing code. For example in AuthServices that supports both IIS and Owin hosting, it’s not acceptable to reference
System.Webfrom most parts of the code.
A Gift to Who?
Finally I’d like to mention the view of pull requests as gifts. I totally agree with viewing pull requests as gifts. But not only a gift from the contributor (sending the PR) to the maintainer (receiving the PR), but also from the maintainer to the contributor.
I give you this code that I have written to improve your project.
I will take care of your code by reviewing it, maintaining it and take responsibility for any bugs in it.
This post was inspired by a similar post by Mark Seemann. It’s very good (although more lenghty than this), so I recommend reading it.
This is a good write-up, but I wanted to address one point:
> Run any existing tests and make sure you didn’t break anything.
If you’re maintaining an open source (or any) project, you should make sure that it’s obvious _how_ to run the existing tests. Most projects do well here, but I’ve seen the occasional one where it’s so hard to figure out how to run the tests that either I don’t bother contributing, or I contribute and keep my fingers crossed.
If there are any integration tests that require external dependencies, this should be spelled out; and this should either be scripted, or the tests should assert their dependencies during setup.
I just recently went through some internal projects updating the top-level README files to make sure this was obvious to new contributors.
I completely agree with you that tests must be simple to run and that they should work out of the box. That’s why I stick to the built in MsTest runner instead of using something better like XUnit. That’s why I make sure the integration tests are self contained in the project.
But all of this is really a different discussion. This post focused on the contributor and what I expect from a contributor. Maybe I should write another post with a list of what a contributor should expect from a maintainer.