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.