Go Diagram

Interesting module. Thanks for contributing it. There are just a couple minor adjustments that I'd like to see implemented.

The define constants should be namespaced. For example, instead of define('DIAGRAM_FILTER_TAG', 'godiagram'); you should have define('GODIAGRAM_FILTER_TAG', 'godiagram');

See the section on comments in http://drupal.org/coding-standards/#naming

The content of the function documentation comments are good, but the documentation is missing for the function arguments and return values. See http://drupal.org/node/1354#functions.

In function godiagram_process_diagram_text(), you should use file_default_scheme() rather than having a hard coded "public://" for those cases where the user has configured the private file system as the default.

There are a few things that can be done to improve this module after passing it through pareview.

Your results from running through it are here: http://ventral.org/pareview/httpgitdrupalorgsandboxmarshmn1602068git

Is it not better to use date types on return as it presented here:
http://drupal.org/node/1354 (Data types on param/return)

Maybe it will be batter to provide step-by-step installation instruction to those for whom "Text format" is not well known.

Also on my configuration this plugin is not working, with no errors (just about deprecated split()).
I posted:
[godiagram]
$$ A joseki mistake
$$ ------------------
$$ | . . . . . . . . .
$$ | . . 8 . . . . . .
$$ | . . 4 5 . 2 . 3 .
$$ | . . 6 1 . . . . .
$$ | . . 7 a . . . . .
$$ | . . . . . . . . .
[/godiagram]
and get only "A joseki mistake" in node content.
Will try to figure out why it's not working.

Update:
The issue was in wrong "Text format" settings that I made.

Update2:
I can't find any similar module.
This module use SL Txt 2 PNG library that is under GPL licence. According to http://drupal.org/node/422996 you may try to commit this library with you module with some changes to avoid error with deprecated split() function. Or you may try to contact with library's developers with your fixes of their code.
Can't find any security issues.
Module accords to Drupal coding standards.

This looks RTBC to me. Very interesting module. I love functionality like this.

Here are my manual review notes:
1. Module is not a duplicate. Very interesting module!
2. 3rd Party Libraries are included with libraries module. Its a shame that
the split() issue can't be fixed in the library.
3. Module contains good documentation and a great demo.
4. Module contains a good example for testing output as well.
5. Module contains good comments in code.
6. Module passes coding standards.
7. Module is using Drupal APIs correctly.
8. I do not see any security issues.

The one thing I would consider changing in the future is to put an underscore in front of the internal, helper functions so that they are not mistaken for drupal hook functions. For instance rename godiagram_load_libraries() to be _godiagram_load_libraries(). But that is a minor issue and everything looks solid.

Thanks,
Bart

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...rupal-7/sites/all/modules/pareview_temp/test_candidate/godiagram.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   106 | ERROR | ISSET keyword must be lowercase; expected "isset" but found
       |       | "isSet"
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

manual review:
godiagram.module: constants should be at the top of the file.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I'm going to give a +1 on the RTBC as well.

There is one very minor issue that I don't think should prevent the release. In godiagram.module line 238 you have an empty else {} block that should be removed.

Also as a future enhancement, I would suggest implementing hook_requirements() to run a check to verify that the "SL Txt 2 PNG library" library is installed correctly. See http://api.drupal.org/api/drupal/modules!system!system.api.php/function/....

Regarding hook_requirements and libraries module, have a look at #1600676: libraries_get_path() is unsafe in hook_requirements() in install phase, and will cause installation to fail since the libraries_get_path() function might not be available at install time.

Thanks for this new module :-)
Happy Drupaling!

.install: Don't use concatenation ('string' . 'string') within t() function!
The translation system at localize is only able to find and translate complete, static strings (eg. t('I am translatable') - t('I am' . 'not'))

But there are no serious issues

Thanks for your contribution!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.