#patch quality

I was reading Mike Hall's post (and I consider him a great friend, so please don't take this as an "attack" on this views - in fact, we agree with most (but not all) of what he is saying), and I thought I'd try to give my take on the issue of patch quality - but from a project maintainer's point of view.

This mostly steamed from a conversation on IRC about patch quality, so this is not actually directed at any content of his post, I'm mostly stating where this post came from. Mike's post was more about maintaining your own project and when to take criticism and when to ignore it.

Before I begin, know that these standards apply for established and structured project, not for the fun little side hack, or the rapidly developed project "X" where you're pushing for a 1.0 before anything else.

Patches are a very important part of any project's lifecycle. It's also important that any project be absolutely clear on where the standard is maintained.

When I write patches, and most absolutely when I review them, I ask myself three questions:

Is it clean?

Is it complete?

Is it correct?

If a patch is clean, correct and complete, it's generally acceptable.

Here's what I mean by those criteria (which, are all judgement calls, I might add)

Is the code clean?

Is the code in line with the project's coding standards? Spacing? Variable names? Is it free of any sort of license issues, and are all the authors attributed? Is it as concise as it should be, or is it too concise?

This covers the "look" and "feel" of the code. Does it blend into the codebase, can you tell who wrote it?

is the code complete?

Did you include examples in the comments, or perhaps some example files. Did you include all the changes you should have? Have you taken the time to ensure it's clear from the outside? Have you finished the docs for it? Have you included all your unit tests?

This covers how well it will integrate into the project, and help eliminate the "bus" factor. Remember, if you're sending in a patch, you don't have commit rights (generally), so the maintainers have no idea how long you'll be around - or even if you'll stay.

is the code correct?

Have you tested it? Does it actually implement everything it says it does (if it's, let's say, a .desktop parser, does it cover every single aspect of the spec? Can you pass it files that will break the parsing?), and if not, why not? Will it break in any non-standard use cases?

This covers if the feature is really ready to be merged in. If you implement 50% to get a single feature, that's pointless, just finish the job while you're in there.