Wiki article on code review

[foxfirefey]

Soooo tonight denise mentioned that mark makes a sad face every time he comes home all ready and rarin' to code only to find a gazillion patches to review. In order to try and spread this burden around, I've attempted to write a guide on the Code Review process so that more people can help with this important chore. I'm posting it for review and discussion, to make sure the instructions in it are sound.

Comments

[mark]

Style compliance -- I only reject if there are other material concerns with the patch. Usually. Some people have been around long enough that I'll kick back patches for style, but generally speaking, it's more frustration than it's worth to tell people to move spaces around if that's the only problem. Especially for newer devs.

The other comment is that you talk about testing the functionality: honestly, it's only about 5% of patches that I test. The only "testing" I do is that I restart my Apache and make sure that the code doesn't cause compilation problems.

A code review is supposed to be of the code: so actually digging in to read the code, see what it's doing, thinking about what other interactions it might have. You also have to keep in mind issues of security, input sanitization, various ways web sites can get owned, and all sorts of things like that. It's not just about testing: the patch might work, but it might also open us up to SQL injection or account takeovers.

Part of the code review process is also determining whether or not something SHOULD be committed. Generally speaking, new features need to be checked off by either Denise or myself (preferably one of us comments on the bug). Massive changes to an existing system should also get checked off.

Maybe more feedback later, gotta run now.

[foxfirefey]

Okay I am FINALLY getting around to this--

I guess the thing to ask is: what chores of code review would you feel comfortable having verified by others, and how could the system support this? For instance, maybe a Bugzilla tag "prereview" that gets bumped up to "review" or something. Or a "checkcompile" or...something. I'll note that I only meant the code review to apply to bugs that have been accepted as things that are going into the codebase, bug fixes and new features alike.

[zorkian]

Oh, I'm totally cool with someone diving in and doing a full review on anything they're comfortable reviewing. I'm hesitant to have any sort of officially tiered review system as that might slow down reviews in general (or give me an excuse to ignore things that aren't prereview+).

Maybe encourage people to do mini-reviews for functionality and compile checking and such. They can poke at it, and if it looks good, just comment to that effect - else, mark it review- with the problem they found.

[alierak]

I would also appreciate a note on what to do with patches in bugzilla. I was submitting one tonight and could not make heads or tails of the " ", "?", "+", and "-" options available for commit and review in the bugzilla attachment form. When I submit a patch, do I set any of these flags? What's a requestee? Etc.

[alierak]

(Or I could read the doc on dev patches instead...)

[mark]

Documentation is overrated!

I know, it's tough, I've had to change my mental model of how LJ did "volunteer development" a hundred times. Here we have people who document things, and actually care about making the process easier to get into!