GHOP #101: Add PHP documentation to Code Review and Lightbox V2 modules

This is a GHOP task. See http://code.google.com/p/google-highly-open-participation-drupal/issues/...

Hi,

First off, thanks for your help with this! Review feedback below.

From looking at the examples and descriptions on Doxygen formatting conventions, I've noted the following:

• There should be a blank line between the function description and the parameters list. This is not the case in the function documentation for _coder_settings_form() for example.
• The @param directive is @param $settings, and not @param array $settings.
• The @return directive should be on line by itself, with the description indented on the following line.
• The verb in the function description should be of the form "Do such and such" rather than "Does such and such" (this is opposite to the file description). Using the _coder_settings_form() as an example again, the description should change to something like Build the form array for coder settings.. However, I think the description needs to be altered a bit to make it clearer what the form is for.
• Each sentence should start with a capital letter (done) and should contain proper punctuation like ending in a full stop (missing).

I also have a couple of questions about the content of some of the comments. Perhaps you could explain what they mean:

• "This functionality is factored out in order to facilitate backwards compatibility."
• "@return array for form API mostly complete for Drupal 5."

Finally in your patch for coder.module, you have:

@@ -436,7 +454,7 @@
   if (isset($form_values['#post'])) {
     $settings = $form_values['#post'];
     $settings['coder_modules'] = _coder_settings_array($form_values, 'module');
-    $settings['coder_themes'] = _coder_settings_array($form_values, 'theme');
+    $settings['coder_themes']  = _coder_settings_array($form_values, 'theme');
     drupal_set_title(t('Code review (submitted options)'));
   }
   else {

The difference between the two lines is that you've added an extra space before the equals sign. Neither version breaks the coding standards, but I would ask that you limit your changes to just the documentation so it's easier to review (unless you do actually find a bug!).

Oh I'd also like to point out that documenting the "warning" functions in the coder/includes/ files is unnecessary and it's also not required to document the functions in the coder/tests/ files either. However, you do have to document the files themselves and all non-warning functions in the includes files.

Good luck with the rest. Looking forward to reviewing your work. Oh, and if you have any questions, don't hesitate to ask. I'm also on the #drupal-ghop IRC channel a fair bit.

Cheers,
Stella

Heya,

Ummm I didn't spot this earlier, but is this a patch against Drupal 5 or 6 versions of the module. The task did say Drupal 6.x, but if you've gone ahead and done all of this for 5.x and are doing the same for lightbox2, then we'll just stick with 5.x then.

Other notes:

• Minor: missing or invalid punctuation in descriptions for coder_settings_form_submit(), theme_coder_checkboxes(), _coder_settings_array(), _coder_read_and_parse_file(), do_coder_review(), _coder_error(), _coder_search_string(), includes/coder_security.inc, includes/coder_style.inc, coder_format_file(), tests/coder_6x.inc
• The @note, @todo lines, should probably be separated from the function one-line summary description by a blank line. This affects _coder_get_default_settings(), coder_page_form_validate(), coder_page_form(), do_coder_review_regex(), _coder_error(), do_coder_review_grep(), do_coder_review_grep_invert(), do_coder_review_callback() and possibly a couple of others.
• You need to write a brief description of the "reviews" and "callbacks" functions in each of the includes files, e.g. in includes/coder_6x.inc you need to write function descriptions for coder_6x_reviews() and _coder_6x_callback().
• tests/coder_5x.inc file is missing its @file description.

I'll get back to you when I've finished reviewing the contents of the comments themselves. Thanks!

Cheers,
Stella

Coder review

General

• Ensure all inline comments (not necessarily added by you) have correct punctuation.

Tests directory

• In tests/coder_6x.inc, the @file directive needs to be on the line after the /** and there are some missing periods.

Scripts directory

• In scripts/coder_format/coder_format.inc, need to separate function summary description from @note line for coder_order_processors().

Includes directory

• In all of the includes files, please ensure the @file directive is on the line after the opening /**. There are also some missing periods.
• In includes/coder_style.inc and includes/coder_security.inc, make the file descriptions clearer. For style, it should specify the Drupal "coding" standards, while for security it should specify Drupal "security" standards.
• For all of the include files, it might be an idea to add the URL for the appropriate section of the handbook. Other than that, the content in these files is fine.

coder.module

• Please ensure the @file directive is on the line after the opening /**.
• Add into the file description, the missing links to the appropriate sections of the handbook for comment standards, sql standards and 5.x to 6.x conversion.
• coder_form_alter() - add in more detail on what changes it makes to which form. Basically it adds a "Code Review" link to the module description text (for each module) on the "Administer >> Site Building >> Modules" page. This allows users (with appropriate permission) to run a code review on a particular module.
• _coder_settings_form() - your description of the arrays confused me at first. Where you say "in form", I initially thought of the form itself. It might be better to say "in the form" or "in the format" or "the array takes the form of". Maybe also add a more detailed description explaining what the form is used for - i.e. basically setting the default reviews and the default modules/themes to run them on. It doesn't run the reviews here, just sets the default form settings for a code review.
• If this is a patch against Drupal 6, why do coder_admin_settings() and coder_settings_form_submit() refer to Drupal 5?
• _coder_settings_array() - function description has an extra invalid period.
• coder_page_form() - it doesn't explain what the function does, i.e. the "coder" page where you can select reviews and the modules/themes to run them on.
• In the descriptions for some functions, it would be better if you didn't say "See function ayz() for arguments." but instead just gave the details again. It's less hassle for the end-user reading the API documentation this way. Functions affected by this are do_coder_review_grep(), do_coder_review_grep_invert(), do_coder_review_callback(), _coder_error_msg(), _coder_page_form_includes(), _coder_severity_name(), do_coder_review(), do_coder_review_regex().
• _coder_error() - you're right, the $original parameter isn't used. It probably was previously. I'd just update the description to say obsolete parameter or something similar and we can remove it at a later date.
• From the final patch, I would remove any @todo messages you've added that weren't there previously. If you feel that there is code there that you wish to improve on, then perhaps submit a patch / task to the coder project issue queue.

Lightbox2 review coming shortly!

Thanks,
Stella

Lightbox2 review

• lightbox2.install - I think the @note directive for lightbox2_install() is incorrect, as that function prints a message to the logs.
• Both lightbox2.install and lightbox2.module are missing the @file directive.
• Ensure all inline comments (not necessarily added by you) have correct punctuation.
• lightbox2_field_formatter_info() - change "Adds" to "Add".

Looks pretty good!

Cheers,
Stella

Hi,

Those look great! Thanks for all your hard work.

There's only one minor problem left and affects both patches. It's not a biggie, but may affect the API parsing of the files, not sure. According to Doxygen formatting conventions, the @file directive should be on the line after the opening /** and the file description should be on the following line, for example:

/**
 * @file
 * My one-line summary description of the file.
 * 
 * A more detailed description of the file (optional).
 */

I think you have a blank line between the @file and the one-line summary description in the majority of cases. If you fix that for each fiel and attach the updated the patches, then this can be marked ready to commit. :)

Thanks again.

Cheers,
Stella

Thanks ezyang, that's perfect. I've applied the lightbox2 patch and checked it into CVS. Assigning this task to the coder module until it also gets committed.

Thanks again, and good luck with the rest of your GHOP tasks.

Cheers,
Stella

The tests files look like they should be transferable without any problems. Will check the includes files tomorrow.

I've compared both the 6.x tests and includes files with that of 5.x, and also used the new 6.x versions of these files on my 5.x test site and couldn't find any problems. I compared the warnings line by line too to be sure! What discrepancies did you find Doug?

I'm now going to look at providing a 5.x patch of the module file based on ezyang's work.

Cheers,
Stella

Attached are two patches for the coder.module file. One is for the 6.x version - just fixes a couple of commenting errors, while the other is for the 5.x version and applies all the commenting changes.

Cheers,
Stella

Done!