Fix and update code documentation

Here's a re-roll. The patch failed to apply because of a recent commit: #960548: Remove support for template files. The template hunks have been removed in this patch.

Thanks! Most of this patch makes sense. A few details:

+++ skinr.handlers.inc	10 Nov 2010 20:41:43 -0000
@@ -10,14 +10,15 @@
  * @param $op
  *   What kind of action is being performed. Possible values:
- *   - "access skinr": access to edit skinr's selector
- *   - "access skinr classes": access to edit skinr's additional classes
+ *   - 'access skinr': access to edit Skinr's settings.
+ *   - 'access skinr classes': access to edit Skinr's additional classes.

(and elsewhere) Only to be updated if we really insist, but definitely to keep in mind for future patches:

1) No quotes around list labels/keys in phpDoc, see http://drupal.org/node/1354 for details.

2) List item description should start capitalized; i.e., with an upper-case letter.

+++ skinr.handlers.inc	10 Nov 2010 20:41:43 -0000
@@ -33,14 +34,15 @@ function skinr_access_handler($op, &$for
  * @param &$form
- *   - For "form", passes in the $form parameter from hook_form_alter().
- *   - For "submit", passes in the $form parameter from hook_form_submit().
+ *   - For 'form', passes in the $form parameter from hook_form_alter().
+ *   - For 'submit', passes in the $form parameter from hook_form_submit().
  * @param $form_state
- *   - For "form", passes in the $form_state parameter from hook_form_alter().
- *   - For "submit", passes in the $form_state parameter from hook_form_submit().
+ *   - For 'form', passes in the $form_state parameter from hook_form_alter().
+ *   - For 'submit', passes in the $form_state parameter from hook_form_submit().

OT for this patch, but still worth mentioning:

If the parameters of a function need such a description and are effectively different depending on another parameter, then this is a clear sign that a function is overloaded.

In this case, however, $form and $form_state always seem to be what $form and $form_state always are: $form and $form_state. Thus, those @param descriptions are a bit weird. :)

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -85,6 +90,24 @@ function skinr_preprocess(&$variables, $
+/**
+ * Helper function to extract the classes, js and css files, and templates
+ * that we are applying.

(and elsewhere)

1) phpDoc summaries do not wrap at 80 chars, see http://drupal.org/node/1354 for details.

2) "JS" and "CSS" are common acronyms, always upper-case.

3) Source code comments should never refer to "we", because "we" is ambiguous in many ways: a) The function caller b) The function itself c) The Skinr module d) The Skinr developers e) The author of a skin f) ... There is only one exception for valid usage of "we": Dirty hacks. In certain edge-cases, dirty hacks are the only way to get something done in Drupal, and only in those cases, it is valid to state that "we" (the module authors) are purposively hacking around a bad-ass limitation of Drupal.

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -85,6 +90,24 @@ function skinr_preprocess(&$variables, $
+ * @param $sids
+ *   The ID, or an array of IDs, for these settings.

Elsewhere in Drupal, $sids refers to session IDs, but I doubt that we are dealing with user sessions here. However, this @param description only states "ID" without stating what kind of ID is being meant.

If the $sids parameter is also used by other functions, then it would be good to write a standardized @param description that is consistently stated by all of those functions.

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -242,9 +265,17 @@ function skinr_skin_get_files($skins, $t
+ * Helper fuction to add CSS and JS files using drupal_add_css()

Typo in "fuction".

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -242,9 +265,17 @@ function skinr_skin_get_files($skins, $t
+ *   'screen'. Only required if $type is set to 'css'.
  *
- * The function checks an array of paths for the existence of the file to account for base themes.
  */

Never leave stale trailing blank phpDoc lines.

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -288,7 +331,14 @@ function skinr_rule_save($rule) {
+ * @param $rid
+ *   The rule ID.
...
 function skinr_rule_load($rid = NULL) {

1) It's odd that the only (and normally required) argument of a (single) loader function is declared as optional, defaulting to NULL.

2) If this argument default value is intended for any reason, then the @param description should start with "(optional) ". See http://drupal.org/node/1354 for details and examples.

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -330,6 +392,15 @@ function skinr_rule_delete($rid) {
+/**
+ * Determin whether a rule is visible to the current page or not.
+ *
+ * @param $rid
+ *   The rule ID.
+ *
+ * @return
+ *   TRUE if the rule is visible to the current page, FALSE otherwise.
+ */
 function skinr_rule_visible($rid) {

1) Typo in "Determin".

2) Simple 1/0 helper functions like this don't need extensive phpDoc. @param and @return can be summarized in the phpDoc summary, like this:

"Returns whether a rule applies to the current page."

3) OT for this patch: Helper functions like this should be named module_foo_IS_bar(); i.e., skinr_rule_is_visible().

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -370,7 +441,12 @@ function skinr_rule_visible($rid) {
+ * @param $file
+ *   The file to include. The file needs to be prefixed with the
+ *   implementing module's name, but do not include the module name
+ *   prefix (i.e. MODULE.$file).

err, what? LOL! :)

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -583,7 +677,7 @@ function skinr_current_theme($exclude_ad
- * Prepare defaults for skins.
+ * Prepare defaults for skinsets.

I thought "skinsets" would be a term of the past?

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -636,8 +730,11 @@ function skinr_skin_default() {
  * Helper function to process a skin or theme .info file.
  *
+ * @param $info
+ *   An array of data as gotten from a skin or theme .info file.
+ *

Looks like we forgot to remove yet another function...?

+++ skinr.module	10 Nov 2010 20:41:43 -0000
@@ -912,6 +1020,63 @@ function _skinr_fetch_config_defaults($s
  * Execute a module's data handler.
...
 function skinr_handler($type, $op, $handler, &$a3, $a4 = NULL, $a5 = NULL, $a6 = NULL, $a7 = NULL) {

Thanks for clarifying all of those parameters! Definitely helpful for rewriting (which badly needs to happen for this function).

+++ skinr_ui.admin.inc	10 Nov 2010 20:41:43 -0000
@@ -58,6 +61,10 @@ function skinr_ui_filters() {
+ * ¶

@@ -187,6 +194,8 @@ function skinr_ui_filter_form_submit($fo
+ * ¶

(and elsewhere) Trailing white-space.

+++ skinr_ui.admin.inc	10 Nov 2010 20:41:43 -0000
@@ -1061,7 +1102,7 @@ function skinr_ui_import_form($form, &$f
 /**
- * Validation handler for Skinr settings import form.
+ * Validate skinr_ui_import_form() submissions.
  */
 function skinr_ui_import_form_validate(&$form, &$form_state) {

phpDoc summary for form validation handlers should be:

"Form validation handler for foo_bar_form()."

(same applies to submit handlers, just replace "validation" with "submit")

+++ skinr_ui.install	10 Nov 2010 20:41:43 -0000
@@ -7,7 +7,7 @@
- * Implementation of hook_install().
+ * Implementats hook_install().

@@ -18,7 +18,7 @@ function skinr_ui_install() {
- * Implementation of hook_uninstall().
+ * Implementats hook_uninstall().

(possibly elsewhere) Typo in "Implementats".

+++ skinr_ui.install	10 Nov 2010 20:41:43 -0000
@@ -27,6 +27,10 @@ function skinr_ui_uninstall() {
+ * @todo This is a badly converted function it should be
+ *       skinr_ui_update_6001(). Remove this function and implement
+ *       skinr_update_last_removed() instead.

If a @todo exceeds 80 chars, indent following lines with 2 spaces. See http://drupal.org/node/1354 for details and examples.

Powered by Dreditor.

I just spent the last couple of hours looking through the code with this patch applied. I made a few minor spelling and coding style fixes and decided to commit this, because it is an improvement and doesn't need to sit here and hold everything else up while it gets worked on.

http://drupal.org/cvs?commit=453738

With regard to this:

I was documenting as much as possible so that other people can more easily write those patches.

I think the code still has a long way to go, from where I sit, to understand what's going on and to be able to write patches. I think the work on getting the terminology right will help, but the thing that would really help me is documenting what's going on inside the functions themselves. There is not enough of that going on.

In addition, I found quite a few functions don't have undocumented parameters or returns. It's inconsistent so I cannot figure out if there is a pattern I'm missing or this was done for a reason or not, so I'm leaving this marked as needs work.

I guess this is going to be done as we go along, so marking this fixed...