The Good, The Bad, and The Ugly

Diogenes's picture

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.

  1. new modules must install with drush
  2. no code that makes anyone barf
  3. 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

  1. 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.
  2. Becoming an Full Project contributor is not important anymore, and neither is the Drupal Open Source project.

 

Comments

doug54's picture
Hello Dio, Interesting and sometimes humorous comments about Drupal.  I've been looking at it, just for shits and giggles; no production website planned at this point.  I would probably use a professional hosting company anyway, for my future web sites.  But still, I am a nerd and programmer at heart, so I set up a LAMP stack and Drupal on an old emachines notebook computer, just to play around with it.  I'm running xubuntu 12.04 on it (with xfce desktop), and also run Samba server on it.  I've been a linux fan for a long time; ran my first linux around 1995 with slackware, then Red Hat & Fedora.  Just recently have used ubuntu and linux mint.  I noticed that when setting up a new user in the default Drupal site, it reported an error: "Unable to send e-mail. Contact the site administrator if the problem persists.".  In fact, it printed this line twice, so I guess the error was twice as egregious.  I looked around and saw your postings on the Drupal Answers page at http://drupal.stackexchange.com/questions/90322/how-do-you-configure-a-drupal-linux-dev-system-for-sending-email ... I read the exchange between you and the Drupal guys, and I couldn't quite believe how arrogant, patronizing, and dismissive they were towards you.  "Wound up a little too tight", as you put it.  I was impressed with your informative answer and solution to the "the now questionable question", however.  Your answer was the sort of response I would expect from a software organization, open-source or otherwise. Not the brusque and caustic stuff they spewed at you. Since I just have a setup on my local LAN and don't plan to deploy my little test sites to the Internet, it's not that important that I get email working properly with Drupal.  I still have the technical interest in it, so I will continue to tinker with it.  But after reading your exchange with Drupal, I am not at all impressed with their attitudes towards other developers and users of their software.  I may ditch Drupal and experiment with other CMS systems.  Btw I am enjoying browsing around your site.  Excellent job! Regards, Doug Brunelle San Diego, CA (yes, California, "The Granola State" - the land of flakes, fruits, and nuts) db91977@gmail.com  
4 years 10 months ago
augustmccl's picture
While i recommend via up to do with this topic area as surely. Bronchodilators are most useful when there is airway spasm. Burp.
2 years 11 months ago