Accueil > Bugzilla, Mozilla > How to require high code quality without discouraging new or occasional contributors?

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. :)

About these ads
Catégories:Bugzilla, Mozilla
  1. Alex Vincent
    15 mai 2012 à 8:42   | #1

    My response to the outside party complaining about high code quality standards: Get used to it. Seriously, if code reviewers aren’t enforcing high standards, they’re not doing their job as reviewers. Code, once checked in, can be around for years before someone changes it.

    You can use code reviews as teaching moments, though: try to encourage people to do self-reviews, to improve their own best practices, etc.

    Now, if the code quality standards are wrong – that is, if the standards are actually encouraging bad behavior – then the standards need to change.

    Keep in mind the Bugzilla project’s code is used by thousands of people around the world. Your work directly impacts theirs. Compromising on quality is a Bad Idea.

  2. ael
    15 mai 2012 à 8:43   | #2

    I think this tackles the problem from the wrong end. When I was a new contributor, I was happy to learn how my code could be improved. What is important is that reviewers take the time to explain, point out relevant examples and sections of the existing code, and are patient and helpful. This costs time, but less than the solution proposed here, and has more long-term benefits (the new contributor learns more).

  3. tom jones
    16 mai 2012 à 5:54   | #3

    "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. "

    it may take half the time to do it yourself, but that loses on the possibility of a new contribubor doing more and saving more time, in the long run..

    a community is not createt by yelling "hey, come play with us, under our rules", it needs to be nurtured..

  4. 17 mai 2012 à 7:32   | #4

    This was also a component of the feedback I received when I did a more detailed survey of every major developer who ever left the project. Although I wrote this up in a fairly detailed email several years ago, I more recently wrote up the findings in even more detail here:

    http://www.codesimplicity.com/post/open-source-community-simplified/

    The particular issue that your developer was concerned about can be resolved primarily by a more rapid response from reviewers. The "respond to all contributions immediately" and "be extremely kind and visibly appreciative" resolves the problem. If you’re very *fast* with your reviews (every review happens on the day the developer posts it) they are considerably happier with high standards than they are when a patch sits around for several weeks or months and then receives short, entirely-negative comments with no educational value for the contributor.

    -Max

    P.S. Also, as a side note, It actually took us *four* development cycles to clean up the code. The last truly problematic release was 2.16 (which didn’t even have templates, really). 2.18 was the first time code quality started to really matter. 2.20 was my DB backend re-write. But 2.22 was certainly when we started the mod_perl refactoring stuff. And even 3.0 wasn’t nearly as nice as what we have today.

    The manpower problem you’re experiencing now is most likely most strongly related to Bugzilla being written in Perl, a language which few professional developers know anymore.

  5. 28 mai 2012 à 8:47   | #5

    I’ll echo what ael and tom jones have stated. I spend little time in Bugzilla bugs, but from what I’ve seen patches are either dormant, or simply shot down (often repeatedly) with what appears to be negativity (don’t do this, don’t do that).

    Showing appreciation, and some careful hand-holding can go a long way. The Bugzilla developers are busy — we get that. But everyone is busy, it’s not just you. When someone takes the time from their own busy day to download your code, understand it, prepare a patch and submit it, that should be an indication that they have all the right intentions of wanting to help out.

  6. Andrew Esh
    7 juin 2012 à 2:35   | #6

    Imagine if you were running a donation-based software development project, and you tried to make the rule that only donations of $100 or more would be accepted. If someone wants to donate $10, they are turned away. How successful would that be?

    You need to recognize the difference between your internal development standards, and the standards to be applied to external contributions. They are two different things. Accept any patch that comes along, like a small $10 donation. Do not try to make the external contributor responsible for meeting your internal standards (the equivalent of a $100 donation). Do not try to make them a part of your internal process. If they want to become a developer within the project, they will pursue that. At that point you can begin to hold them to professional development standards.

    While I am advocating that you accept every patch, there is no need to apply them in their initial form. Attach them to a bug, and see that the bug gets processed. An internal developer can bring the patch up to professional coding standards. There is no reason to make the external contributor responsible for that, because it drives them away. This deprives the project of potential developers who could help with the workload of cleaning up contributed patches.

    Always be welcoming patches and contributions of time and money, no matter how small.

  7. Joe Greenman
    4 octobre 2012 à 11:36   | #7

    I had originally thought of posting this as a comment to Bugzilla bug 540. I then realized that replying here is more suitable for what is pretty much a personal observation that I feel is more related to the question asked in this blog’s title than it is to the pre-millennial bug’s resolution. Nonetheless, the bug – which I’ve used as a case study – runs through this posting for reasons that I hope will be obvious by the time I get to the end.

    Before I start, let me confess that I’m a 66-year old American who teaches English at Berlin’s Technical University and not a computer person. Having said this, about half of my students are computer scientists, so I’m not completely unfamiliar with the territory. I’ve also been a Firefox beta-tester since it was called Phoenix. Furthermore, in my career I’ve worked in (in alphabetical order) Egypt, England, France, Germany, Saudi Arabia, Switzerland, the US, and Yemen. My teaching has been done in a wide variety of contexts: universities, a hospital, an airport, an oil company, several governmental language-training institutions, top-tier research facilities, etc.

    Now to get to the point: The owner of this blog replied to a barbed-but-not-off-the-mark comment I made about Bugzilla bug 540 – which has existed since 1998 – by saying, "… we tend to consider as spam emails coming from such bugs and sometimes don’t even bother reading them."

    Well, sir, I’ve "bothered" to read your entire blog posting here – and the replies. I contend that the resolution of your issue (which is broader than "the answer to your question”) is not technical in nature but rather lies in the realm of what I’ll term "attitude."

    The breakdown of the replies to the blog owner’s question was revealing: The two Mozilla/Bugzilla insiders defended the fort, while the three outsiders opined in favor of flexibility.

    I’d like to go back to bug 540 for a second. After original expressions of interest over a period of years by Bugzilla Biggies including Dave Miller and MKA, the latter, with no explanation whatsoever, summarily posted "Priority: P2 → P3" (2007-02-27) and then "Priority: P3 → P4" (2008-04-03). His name later pops up a couple more times, as does the name of this blog’s owner. Unfortunately, this bug appears no closer to closure today that it did in 1998.

    Dear Bugzillists: The "it’s my football, so we’ll play by my rules" attitude is well known, not only in the schoolyard but in professional endeavors all over the world. One that quickly comes to mind is international development; nuclear disarmament is another. Fortunately, the world of bug-tracking is less pernicious.

    Anyway, if the Bugzilla insiders feel this attitude helps their product, I guess it’s "tant pis" for me and the others who have indicated they’re not totally happy with the way things operate.
    In closing, as I’ve said, I don’t think the issue of “How to require high code quality without discouraging new or occasional contributors?” has a technical answer. I believe successfully achieving what you seek has to do with a change in your style of communication, a change in Bugzilla insiders’ attitude.

    Sorry for the prolix.
    —————-
    P.S. In regard to bug 540, why not simply press the "WON’TFIX" button (yes I know the Bugzilla version doesn’t have an apostrophe) and put all the people who have tried to get an edit function in Bugzilla since 1998 out of our misery?

  8. 5 octobre 2012 à 2:27   | #8

    Frédéric, n’oublie pas que les gens qui prennent le temps de poster un commentaire, que ce soit ici ou sur un bug, sont passionnés, ils aiment Bugzilla et ils n’ont à coeur que son succès.

    Merci de garder un esprit ouvert.

Laisser un commentaire

Entrez vos coordonnées ci-dessous ou cliquez sur une icône pour vous connecter:

Logo WordPress.com

Vous commentez à l'aide de votre compte WordPress.com. Déconnexion / Changer )

Image Twitter

Vous commentez à l'aide de votre compte Twitter. Déconnexion / Changer )

Photo Facebook

Vous commentez à l'aide de votre compte Facebook. Déconnexion / Changer )

Photo Google+

Vous commentez à l'aide de votre compte Google+. Déconnexion / Changer )

Connexion à %s

Suivre

Recevez les nouvelles publications par mail.