I try to help contributors to make it so their commits can be merged without any changes. For some this can be annoying in the beginning, but the goal is that they learn to write quality patches and that I will have less work getting their work merged. I hope that this is a win-win. There are times where I do some changes (rebasing, small fixes, rewording of commit messages), but I will try to ensure their name is in the Author: field in the commit messages.
As mentioned by sph: some projects have maintainers that have much stricter standards for outside contributors that for themselves. Speaking as a maintainer of a project that accepts contributions through PR requests, I am afraid I do indeed have such a double standard, although I try to avoid it as much as possible. I had to set some rules for myself:
- Contributors must fix outright bugs in their commits.
- Style issues are avoided by having a coding style enforced by a code formatting tool. This saves everyones time.
- If the commit is bug free (passes test suite and code review) and is a net improvement, accept it, regardless of my own plans for the project.
- If there is more that can be done to make it ideal, this can be done in a future pull request, or perhaps I will add those changes later myself.
I also try to hold myself to the same standards as I hold contributors to, and now often create a development branch myself for features so they get the same CI testing as pull requests, although I still sometimes bypass this for quick fixes.
Just wanted to say thanks for sharing these rules, I think they’re super helpful.
Do you have any particular rules about performance? I worry that accepting net improvements on functionality alone could result in “death-by-a-thousand-cuts” performance issues down the line (assume it’s a project where performance is one of the deliverables).
Would you ask a contributor to perform benchmarks or some other performance validation, for example?
It would depend on the project. If performance is important, it should indeed by validated, but then it would greatly help if the project also has a performance test suite that contributors could run locally, and compare their change against the baseline, and a reviewer could then also verify it themselves.
What you do with the performance results really depends on the kind of patch they want to commit. Is it an important bug fix? Then I would get that merged first and worry about fixing performance afterwards. And then you have those patches that improve performance in one area at the cost of decreasing it in another. In that case you might have to use your experience in what is more important to decide whether to accept or reject it.
As mentioned by sph: some projects have maintainers that have much stricter standards for outside contributors that for themselves. Speaking as a maintainer of a project that accepts contributions through PR requests, I am afraid I do indeed have such a double standard, although I try to avoid it as much as possible. I had to set some rules for myself:
- Contributors must fix outright bugs in their commits.
- Style issues are avoided by having a coding style enforced by a code formatting tool. This saves everyones time.
- If the commit is bug free (passes test suite and code review) and is a net improvement, accept it, regardless of my own plans for the project.
- If there is more that can be done to make it ideal, this can be done in a future pull request, or perhaps I will add those changes later myself.
I also try to hold myself to the same standards as I hold contributors to, and now often create a development branch myself for features so they get the same CI testing as pull requests, although I still sometimes bypass this for quick fixes.