foxfirefey (
foxfirefey) wrote in
dw_dev2009-04-03 03:37 am
![[personal profile]](https://www.dreamwidth.org/img/silk/identity/user.png)
![[site community profile]](https://www.dreamwidth.org/img/comm_staff.png)
Entry tags:
Wiki article on code review
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.
![[staff profile]](https://www.dreamwidth.org/img/silk/identity/user_staff.png)
![[staff profile]](https://www.dreamwidth.org/img/silk/identity/user_staff.png)
no subject
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.
(no subject)
(no subject)
no subject
(no subject)
(no subject)