MergeDocx

Hey Changvn

Thank you for your module proposal.

I found a few things incorrect with your code.

Let me start with Drupal coding standards.

It is always a good idea to run an automated review of your code before posting a review request.
That way you can find and fix the errors beforehand and speed up the reviewing process.
I've setup an automatic review that you can find here: http://ventral.org/pareview/httpgitdrupalorgsandboxchangvn1666360git

Some points that the PAReview catched:

• Don't include a LICENSE.txt with your module.
• Try to watch you 80 column per line length, some of your lines are really long
• Always include a new line at the end of your code files
• Mind unnecessary white spaces inside array declarations and function calls
• Also mind trailing white space at the end of lines
• Make sure that your in-line comments start with a space between // and the sentence. Also end them with a ful stop (.)
• Try not to use camelCase with variables of function names, always use underscores
• ...

Then a few things about your GIT repository

• Please make sure that you create a 6.x-1.x branch in which you put all your development code.
• Clean out your master branch to only include a README.txt saying that people should check out the 6.x-1.x branch.

And finally my manual testing report...I'm afraid I did not get very far.
I successfully installed the module on a clean Drupal 6 tree and noticed I received a new link in the top of my admin menu called "Merge DocX". It links to mergedocx/download, but I get a 404 not found error.

It is not clear to me what I should do now, or if I need to set another setting for this to work.
The only settings that I found tells me where the template files are uploaded, and that directory is fine.
Did you test your module on a clean Drupal 6 (6.26, latest in my case) installation before posting?

Cheers
T

Manual review of your module:

- You're inconsistent in the syntax-use of the declaration of your arrays:

$form['mergedocx'] = array(
     '#type' => 'fieldset',
     '#title' => t('General config'),
     '#collapsible' => TRUE, '#collapsed' => FALSE,);

vs.

$ds['nid'] = array(
     'name' => 'nid',
     'type' => 'node',
     'multiple' => 0,
     'view' => array($node->nid),
 );

The second way is the most common way and also how the code conventions say it should be done.

- You are calling exit() in a menu-callback: mergedocx_mergefield. Not really good practice.

- Some new lines can really improve readability a lot; you might also want to add a description in the docblock of your really heavy functions.

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