Drupal Code Reviews
My first request for a review was on Aug 3, 2011. The first review came 66 days later. It was taking so long, I had stopped checking the dashboard and didn't notice a review had been completed until a month later.
It was an excellent code review. I set out to correct the things mentioned and made some improvements to support other popular modules.
The second review came in only 17 days. This coincided with the appearance of Code Sniffer, also known as PAReview, along the most unreadable CAPTCHA ever.
There were nothing serious this time, just styling errors. I was encouraged by the quick turnaround; continued to develop the concept; added an 8.x branch and cleaned up the numerous PAReview errors on 3 branches of code 6,7 & 8.
The third review came 30 days after the next request. It was also an excellent review and made some good suggestions. There were some non-operational errors mentioned and a few alternate approaches or functions calls suggested.
I tried out all of the suggestions but they did not pass my own functional testing. By this time I had set up a multi-site test platform with a fresh install for each version. The module was tested with at least 10 different themes (except for the 8.x branch).
I found it necessary to write some custom functions because all Drupal API suggestions offered failed in certain install scenarios.
On January 31, 2012, I set the status back to 'needs-review' for a fourth review request. That was a remarkable day because I had a review 42 minutes later!
The reviewer had set the status back to 'needs-work' based on some minor PAReview errors. A Git admin corrected that 22 minutes later with this message:
please don't block deeper reviews because of little formatting errors ;-)
17 minutes after that the same reviewer apologized and said he tested the 6.x branch and it worked fine! He did not change the status to RTBC. I can't blame him because I didn't know what the protocol was myself. So the project status remained at 'needs-review'.
Reviews #5 and #6 came 17 days later and less than 2 hours apart.
The Good
The first was a manual review that basically confirmed the module worked and had some helpful suggestions. I responded to his post and he, in turn, responded to mine.
The Bad
The second reviewer that day complained the module did not install with drush (this is a requirement now?); suggested that I should instead take over a module abandoned 4 years ago; complained about the use of camel case notation on local variables; spoke of 'crazy' code; suggested an alternate drupal function that was totally inappropriate; and then finally recommended that I test my code more thoroughly before I commit it the next time.
Suppressing my natural instinct, I responded with a plea for help. I develop on a WAMP stack so I don't use drush. I asked that he do a conventional install instead of a drush install and made a small change to the install code hoping that it might satisfy the whatever drush configuration he had. Then I set the status back to 'needs-review'.
That was back on February 29. I never got a response.
The Ugly
Review #7 followed 63 days later (May 2). The reviewer opened with a flattering remark and then proceeded to feed the module through the shredder. It was largely a rehash of review #6, with a pet peeve tossed in, and this charming rejoinder
while (TRUE) {"statements make me wanna barf :) }"
Note the clever happyface emoticon.
None of the points made in this review had anything to do with utility, function or performance. The fact that the module worked (according to two previous reviews) did not matter at all. Previous postings were mostly ignored.
The objections were all about style and pedantry; about NOT doing things the proper Drupal way; about the proper Feng Shui of braces, parentheses and indentation; about the illegal use of camel notation; and *_private* functions.
Oh yeah -- and the module did not install with drush. There was a theory posited as to why, but then why bother looking into it more? It does not install with drush, loser. Stop wasting my time.
I was so mad at the last review that I did not answer for four months. That did not help. I am still mad.
This process is not like any other peer review process. It takes much longer than it should and there is not much an applicant can do against a bad review.
A random reviewer can walk in, do a quick and sloppy job, pontificate about the Drupal way for some review points or just to hear his own voice; send the applicant to the back of the bus; and never even bother to respond.
There is no automatic reset if a reviewer does not respond to an exception call by or a question by an applicant.
Judgments can be rendered by nit-picking code monkeys. Then there are unwritten rules. i.e.
-
new modules must install with drush
-
no code that makes anyone barf
-
always use drupal_{wtf} API functions when available because Drupal API functions are always the best.
The rules can also change whlle you are waiting in the queue -- proper punctuation on comments, unix file format, no trailing spaces, no tabs, mind the gap, blah blah blah.
Lessons Learned
-
One can learn a lot from the code review process, no matter how much experience you have. Code reviews are not easy (at least they shouldn't be) for either the reviewer or the reviewee.
-
Becoming an Full Project contributor is not important anymore, and neither is the Drupal Open Source project.
Comments
doug54
augustmccl