patch review guidelines

[kareila]

My initial revision of Dev Reviewing Guidelines is up on the wiki. Check it over and see if it looks good.

Please consider being a code reviewer! It's way easier to test code than it is to write it, but for some reason we always seem to have a backlog of unreviewed patches sitting around. If patches go through peer review, that reduces the workload of the handful of developers with commit access.

Comments

[exor674]

"Make sure the commit flag on the patch is set to '?' so that it shows up in the queue of commit requests."

Sometimes the patch maker will make a patch intentionally w/o commit?, so the reviewer shouldn't set it

[kareila]

Qualified statement with "unless indicated otherwise."

Thanks for the feedback!

Reviewing patches

[cesy]

There are some very cool things sitting in the queue for review, but most of them I don't understand enough to be confident reviewing. Is there a way of doing a "looks okay on preliminary review, but someone who knows more than me should probably check it"?

Re: Reviewing patches

[kareila]

If you have a development environment, you can apply the patch there and just check that it behaves the way the author thinks it should, even if you don't understand how it works! Just leave a note to that effect if you review+ the patch. Don't worry, at least one other person will eyeball the code before it gets committed.

(If you don't have a development environment, you know you can get one really easily, right?)

There have been some patches with very good looking code that I have left alone just because I didn't know enough about the part of the site they were changing to test them with confidence.

Re: Reviewing patches

[afuna]

For those, you can leave a comment without setting a flag. Something along the lines, "I took a look at this, the style is clean", or "X could be a concern", etc.

Just make sure to note explicitly which aspects you've looked at, and any comments you leave will help ease the load!

(PS. \o/)