As a maintainer of an open source project which strictly enforces coding style and 100% test coverage, how do I deal with enthusiastic contributors who write terrible looking, untested code (that might work fine), while the contribution guidelines (and even the readme) very clearly mentions how your code should be formatted/tested.
1
-
3Is there anything wrong with using a saved reply (e.g on github blog.github.com/2016-03-29-saved-replies) stating "please make sure you have read the contribution guidelines before contributing" ?– ESRCommented Oct 9, 2018 at 1:39
Add a comment
|
1 Answer
Define objective criteria that any contribution must fulfill. Automate checks for these criteria, using unit tests, linters, code coverage tools, …, then automatically run these checks for each PR as part of the build process / unit tests. Fortunately, there's a large set of such services that are available at no cost for open source projects.
If a contribution fails these criteria, decide how much effort you want to invest into that PR and the PR's author.
If a contribution does not make any sense for your project, kindly say so. For example:
Thank you for working on this idea, but I won't merge it. Adding this feature means we won't be able to do X in the future, and I'm not yet sure whether that's the right decision. In the future, if you have any ideas please open an issue first so that the design can be discussed.
It might be possible to prevent some weird ideas by publishing a project vision and roadmap that outlines where you want the project to go. This will make it easier to say no to contributions that are not aligned with this vision.
If the contribution is irredeemable in its current form, close the PR with a kind message that refers to your objective criteria/your contribution guidelines, but possibly leaves the door open. For example:
Thank you for your contribution, this looks like a great addition for {project}. Unfortunately, the tests on {CI service} seem to be failing so I'm closing the PR for now. Please let me know when it is fixed so that it can be reopened and merged!
Depending on your patience also offer some help, e.g. a chat room where you can be reached.
If the author is willing to make changes or when you want to grow them, comment with specific requests for improvements. E.g. GitHub's code review feature can be great for this. If necessary, consider providing (links to) tutorials and how-tos. For design-level problems, I've found it useful to NOT say “do this instead” (because my ideas are often not that great either) but to ask “what happens if in case of X?” or “I don't understand why this algorithm is necessary. Can't we just add the item to the list?”.
If the author is unavailable but their change is valuable, consider carrying the PR and fixing any problems yourself. This often takes less effort than writing about your concerns and waiting for responses, but doing work yourself instead of involving others makes your community poorer.
If in doubt about a PR/feature, say no. No is temporary, yes is forever. This is not personal, this is about keeping the project viable. After all, a feature doesn't just cost effort for the implementation, but will cost effort for maintenance and user support in the future – effort which you as the maintainer might have to provide. Setting boundaries and not taking on work that you don't want is important.
Consider also reading Jessie Frazelle's post The Art of Closing.
-
3+1 cause that's exactly what I wanted to hear :p. I am enforcing automated checks for test coverage and code review, but till now I haven't found something that really checks code style. I'll mark this accepted if there are no other opinions on this one.– pulsejetCommented Oct 8, 2018 at 13:53
-
2
-
2@PulseJet I use flake8 (which is mostly great, but limited) and pylint (which has many false positives, so do write a config file to disable irrelevant diagnostics). If you like TypeScript, you might also like Python's mypy type checker, especially when you only support 3.5 or later.– amonCommented Oct 8, 2018 at 15:49
-
2It’s worth noting that adding coding styles witch are too restrictive might drive people away from contributing, which might be a problem if your project doesn’t receive many PRs in the first place.– SteveCommented Oct 8, 2018 at 19:31
-
2@PlasmaHH There has to be a balance. Sure, projects that close PRs with “Fix your code first!” responses come across as dickish. But every PR can easily cost a maintainer over an hour of work just reviewing it and writing a response, even if the response is a “no”. Carrying a patch can cost multiple work-days. It is therefore irresponsible to swamp maintainers with half-baked PRs. Part of my answer is about setting a sufficiently high bar for PRs, which reduces the maintainer's cost. If you have a prototype, just link to it in an issue comment instead.– amonCommented Oct 10, 2018 at 10:24