How to require high code quality without discouraging new or occasional contributors?
During the week-end, I received a pertinent email from a former Bugzilla developer who replied to an email I sent to all reviewers about the pretty low activity in the Bugzilla project during the current development cycle. He argued that one of the reasons which made him go away, and which probably took some other contributors away as well, is our high code quality standards we have in this project. The point is that we deny review if submitted patches do not follow our guidelines or are poorly written compared to what we expect in our codebase. He suggested that reviewers should accept and commit lower quality patches, and file follow-up bugs to clean up the new code. I then brought this discussion on IRC with other core developers, and we realized how hard it is to define the right level of code quality. The problems are:
- Some new features require some rewrite of some parts of the code to be easily extendable in the future and/or to reuse some existing code. These features could also be implemented without the code rewrite, which is easier to do for the contributor, but then would be harder to maintain, or would require to duplicate some existing code, which we try to avoid as much as possible. This is how Bugzilla 2.x was written, and it took us two full development cycles (Bugzilla 2.22 + 3.0) to clean up and rewrite the codebase to be able to implement new features in a reasonable way (the 2.x codebase became too ugly and complex to easily implement anything new on top of it). So the risk to accept such patches is that the codebase would become more messy again. But on the other hand, the risk is also to make contributors leave if we keep our high standards, because a rewrite is generally not something trivial nor exciting, and contributors generally do not understand why we reject a patch which does the job.
- Accepting patches which are of lower quality and filing follow-up bugs to fix the new code before the next release seems reasonable only if the community around the project is large enough to have the manpower to do the job in the expected timeframe. The current Bugzilla team is very small, maybe 4-5 core developers + some occasional contributors helping with some not too hard bugs. This isn’t large enough to expect these follow-up bugs to be fixed promptly. But we also cannot release a new version with poor code in it, some of the reasons being security implications, performance issues, or possible regressions in some use cases. As the community is small, fixing these bugs would delay the next major release a lot, which is not a desirable effect.
- Depending on how the submitted patch is written, it would take twice the time to review + rewrite the new code to match our standards compared to the time it would take for a core developer to write the patch himself. The lack of time being the main enemy, I personally wouldn’t want to spend more time on such or such feature because it’s not written as we would like it to be. The time spent to rewrite bad code means less time to work on other stuff.
So what’s the right threshold to not make new and occasional contributors go away without badly impacting our codebase? Which rules do other Mozilla and non-Mozilla projects use to solve this problem? Please share your experience with us.