Bastlynn [bastlynn]

Let's keep the Drupal 6 version for the review.

1. 
       if( (!$teaser && $op == 'insert') || (!$teaser && $op == 'update') ) {
           if($node->dice_rolls['roll_command'] > 0){
               $did = _dice_roller_parse_roll( $node->dice_rolls['roll_command'], $node->dice_rolls['roll_notes'] );
               _dice_roller_set_origin($did, $node->nid, 'n');
           }
       }
   
       if(!$teaser && $op == 'delete') {
           _dice_roller_delete_roll($node->nid, 'n');
       }
   
       if(!$teaser && $op == 'view') {
           $node->content['body']['#value'] .= _dice_roller_get_roll($node->nid, 'n');
       }
   

   See the Drupal coding standards to understand how a module code should be written. I will not report other code that is not compliant with.

2.     if(strlen($pid)==0){
           return '';
           exit;
       }
   

   If the function is returning to the caller, then exit is not executed.

3.         '#description' => t($help_text),
   

   The first argument for t() needs to be a literal string; differently, the script that extracts the strings to translate is not able to extract the string.

4. The help text you used is the same reported in http://invisiblecastle.com/roller, which says portions of the text are copyrighted from The OpenRPG Project; you are using the same portions of text without reporting the copyright holder.

I forgot to change the status.

Rather than repeating text that is available from a third-party site, it would be better to put a link to the page which gives the description you copied in the module. Actually, if somebody doesn't know what he should write (or the functions he can use), it is more probable he should not be using the module. :-)

It makes sense.
Still, the text should be changed. I don't think function names used by some kind of scripting language can be copyrighted, but the rest of the text can be; it just a question of saying the same thing without to use the same words used by the other web site.

There are no installation instructions for this module, and no configuration option. What is one supposed to do with it once it's installed?

Thanks

Ok, I see the Roll Dice option in my comment form. So I tried it out on my D5 RP site, and got the following error when trying 1d20+5 as the roll.

Fatal error: Call to undefined function _dice_roller_set_origin() in /home/hdrpgco/public_html/drupal/modules/Dice_Roller/dice_roller.module on line 119

One thing I noticed when I tried to install this, there was a conflict since the Drupal 5 and 6 versions are in the same folder structure. I had to delete the D6 version to get it to install, which it then did without error. So maybe something got hosed on the first install attempt?

I grabbed the one in comment #2

@Bastlynn: We review just a module per applicant; as you noticed, I am reviewing the Drupal 6 version of the module.
It would be better to give to the archive the same structure the archive generated from the packaging archive running on Drupal.org would have.

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.

Hello, and thanks for your reply. You need to re-apply for a CVS account as your application has been already rejected.

Is that for D5 or 6?