Part of meta-issue #1310084: [meta] API documentation cleanup sprint and continuation of #1327484: Clean up API docs for book.

This issue is focused on further changes to bring Book module closer to D8/D7 documentation standards. This issue, for instance, will ensure that there are no missing @param or @return directives from docblocks and that the various Test files are in accord with http://drupal.org/node/1354. There also appears to be at least one missing @file directive.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Status: Active » Needs review
FileSize
922 bytes

Attached is a small patch from starting to perform a full review of the Book module for compliance with D8 documentation standards.

If interested, please add to this patch as one does a full review of each of the files in the Book module directory.

jhodgdon’s picture

Status: Needs review » Active

This patch has no problems, so I committed it to 8.x and the .js file part to 7.x (the css file does not exist there). Thanks! Leaving open for further work if necessary.

Lars Toomre’s picture

Status: Active » Needs review
FileSize
15.45 KB

Here is a patch that contains changes from doing a full review of the Book module (including its Test class).

jhodgdon’s picture

Status: Needs review » Needs work

Thanks, most of this looks good! A few updates needed:

a) In book.admin.inc:

- *   The form that is being modified.
+ *   The form that is being modified, passed by reference

Needs . at end of line.

b) in book.install:

/**
- * Move book settings from variables to config.
+ * Move Book settings from variables to config.

Only capitalize Book in "the Book module". I guess here it should say "Move Book module settings...". Also check other capitalization changes, such as in the BookTest.php file.

d) book.module

+ * @return
+ *   A Boolean TRUE if the current user has the requested permission.
+ *
  * @see book_menu()
  */
 function _book_outline_access(Node $node) {

By convention we do not include return value docs for hook_menu access callbacks (there are 3 in this patch).

e) book.module

/**
  * Processes variables for book-node-export-html.tpl.php.
  *
- * The $variables array contains the following elements:
- * - node
- * - children
+ * @param array $variables
+ *   An associative array containing the following keys:
+ *   - node

- children got left out - was that intentional?

f) BookTest.php

+   * @param array $nodes
+   *   An array of Nodes to check in outline.
    *
-   * @param array $nodes Nodes to check in outline.
+   * @return string
+   *   A customized regular expression as a string.
    */
   function generateOutlinePattern($nodes) {

Nodes should not be capitalized in the @param description? And the @return docs don't actually tell me what the regexp is for -- and of course it's a string (that is how regexps are in PHP), so that's not useful information.

g)_ same file

+   *   (optional) Parent book reference id. Defaults to NULL.
    */
   function createBookNode($book_nid, $parent = NULL) {

id -> ID

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.44 KB
13.32 KB

Addressed issue raised in #4 and found a few others.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Thanks! This one looks good, and I committed it to 8.x. I guess we should port it to 7.x -- careful though, as there are references to the Node class etc. which don't exist in 7.x.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
11.29 KB
jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good to me! I'll get it committed soon. (Only one pass on this one -- great!)

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

It's in -- thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.