The Hoover module (Drupal 7) sends watchdog events to loggly.com and includes a JavaScript logger for scripts that support it.

Project URL: http://drupal.org/sandbox/suboracle/1394372
Git URL: http://drupalcode.org/sandbox/suboracle/1394372.git

CommentFileSizeAuthor
#1 drupalcs-result.txt2.46 KBklausi

Comments

klausi’s picture

Status: Needs review » Needs work
StatusFileSize
new2.46 KB

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

  • Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
  • Remove "version" from the info file, it will be added by drupal.org packaging automatically.
  • ./hoover.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
    function dblog_install() {
    
  • Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
  • Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

manual review:

  • hoover.install: calls to drupal_install_schema() and drupal_uninstall_schema() are not required anymore in Drupal 7.
  • "Drupal.behaviors.exampleModule": the name of your module is not "exampleModule"
  • "define('HOOVER_CRON_FREQUENCY', serialize($cron_frequency));": a serialized constant is ugly. I suggest to define a function that returns that predefined array.
  • "'access arguments' => array('administer site configuration'),": do not use this generic permission, create your own.
  • hoover_watchdog(): $hoover_url is defined far away from its usage, bring it closer to the HTTP request.
  • "'#description' => 'The Input Key provided by Loggly',": all user facing text should run through t() for translation.
klausi’s picture

Category: feature » task
richard banks’s picture

Status: Needs work » Needs review

I have applied all the necessary changes and have tested the module successfully against coder and the Drupal Code Sniffer.

mkadin’s picture

Status: Needs review » Needs work

Cool stuff. I didn't know about loggly, but it seems like a cool service. A bit pricey for my tastes...

Anyway, it looks like you've covered most of the automated review process, but there are a few remaining things to clear up:

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

  remotes/origin/7.x-1.x-dev

Review of the master branch:

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

Also on lines 80->88 of your .module file, you've arranged the array assignment so that the operands line up. I like this formatting, but I'm not sure that it's the drupal coding style. I could be wrong though...

For javascript files getting loaded on every page load (i.e. the drupal_add_js on line 159), you can add them in your .info file for your module. I believe this is more efficient.

Good stuff though!

richard banks’s picture

Status: Needs work » Needs review

Ok all those issues have been corrected as well. Third time's the charm?

richard banks’s picture

Priority: Normal » Major

The code now passes all the tests, what's next?

firebird’s picture

I've reviewed the module, here's what I think:

Licensing: No issues found. It looks like there is no 3rd party code.

Module duplication: There are no other modules at drupal.org that integrate with Hoover.

Overall understanding of Drupal API's: Looks good to me.

Security: No problems found.

Drupal coding standards: PAReview is happy.

Code documentation: There are next to no inline comments in the code, but the code pretty much documents itself.

luxpaparazzi’s picture

Status: Needs review » Needs work

Thanks for telling people about the service with whom your module interacts!
But you should still try elaborating your project page, see:

project page
Please take a moment to make your project page follow tips for a great project page.

https://d3eyf2cx8mbems.cloudfront.net/js/loggly-0.1.0.js'

I'm not convinced hardcoding external web-addresses in your code is a good idea, you should make this modular, so an admin may correct the path if necessary ...

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.


FILE: ...an/www/drupal7/sites/all/modules/pareview_temp/test_candidate/hoover.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 7 | ERROR | There should be no white space after an opening "{"
--------------------------------------------------------------------------------

IMO:

Code too short
This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project.

Even that your code is larger as described above I still find it to short for doing a serious evaluation.

luxpaparazzi’s picture

The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

klausi’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.