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

This issue is focused on further changes to bring Block 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

FileSize
6.58 KB

Attached is just a start of a full review of the Block module for compliance with the documentation coding standards.

Lars Toomre’s picture

Status: Active » Needs review

If anyone is interested in taking this full review further, please feel free to do so.

Lars Toomre’s picture

FileSize
4.46 KB

Added changes to block.api.php file to the above patch.

Lars Toomre’s picture

Hmmm... For some reason #3 was not an addition but completely separate.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patches!

The block.api.php changes in the patch on #3 are incorrect -- see
http://drupal.org/node/1354#hooks

The patch in #1 looks pretty good. A couple of spots could use some attention:

a)

/**
- * Sorts active blocks by region, then by weight; sorts inactive blocks by name.
- *
  * Callback for usort() in block_admin_display_prepare_blocks().
+ *
+ * This callback function sorts active blocks by region, then by weight; it
+ * sorts inactive blocks by name.
+ *
  */
 function _block_compare($a, $b) {

The doc that was there was more compliant -- see http://drupal.org/node/1354#callbacks

b) Why take out useful documentation? It should probably be on @param $variables and reworded, but it's still useful:

/**
  * Processes variables for block-admin-display-form.tpl.php.
  *
- * The $variables array contains the following arguments:
- * - $form
- *
  * @see block-admin-display.tpl.php
  * @see theme_block_admin_display()
  */
Lars Toomre’s picture

Thanks @jhogdon for the comment about the patch #3.

Until your comment, I did not know that the one line directives in *.api.php files were different from normal file docblocks. Be aware that I probably have made the same error in other full review patches that I recently have rolled.

jhodgdon’s picture

Yeah, we have a lot of standards! If you think you've made the same error in other patches, and you know which patches, it would be very helpful if you could fix them... just a thought. :) I'll try to keep a look-out though.

Lars Toomre’s picture

@jhodgdon Thanks. Working with Drupal core commit process is a continual process of learning...

Based on what I remember, there might well be other recent patches that include:
- a) Verb tense fixes in *.api.php files,
- b) Verb tense corrections in *.install files,
- c) The phrase '???' when I knew that a proper text string was required,
- d) Something else ...

As I review the approximately 40 patches that I have created, but have not yet been committed, I will review them for the above criteria.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
20.04 KB

The attached patch builds upon that in #1 and includes the comments from #5. It further includes other changes noted in a full review of all files in the block module, including its Test classes.

Status: Needs review » Needs work

The last submitted patch, 1807652-9-block-docs.patch, failed testing.

Lars Toomre’s picture

FileSize
20.04 KB

Helps if there is not a stray character in block.admin.css... Let's see how this locally untested patch does.

Lars Toomre’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1807652-11-block-docs.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
20.03 KB

Grr... I am having editor problems. Let's try that one more time.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch!

A few things to clean up:

a) book.admin.inc:

+ *   (optional) The name of the theme.  Defaults to NULL.  This variable is not

Lots of extra spaces in this line.

b) My usual comment about not changing the order of @see statements applies. We don't have a standard that they should be alphabetical so it does not bring documentation into better standards compliance to alphabetize them, it just makes more work for patch makers and patch reviewers. Please remove those changes.

c) This type of change is also not necessary:

- * Saves the new custom block.
+ * This function saves the new custom block.

We don't need "this function..." ever to be in function docs really.

d) in block.module - the hook_menu access callbacks by convention should not have @return docs.

e) This is not a helpful change: (block.module)

- *   The theme to rehash blocks for. If not provided, defaults to the currently
- *   used theme.
+ *   (optional) The theme to rehash blocks for. If not provided, defaults to the
+ *   currently used theme. Defaults to NULL.

I actually think it was clearer without that extra "Defaults to NULL" statement, because it already said what was happening if nothing was provided, and this extra thing makes you think "now what is that all about?". Really, adding all the "defaults to xyz" things that you seem to add to every patch doesn't add a lot to the docs -- the function with its default args is right below (or right at the top of the page if someone is looking at it in api.drupal.org). I really don't see that we need to go through every function and make sure this information is added to the documentation -- it's a lot of work for very little value added, and we don't have a standard that says we need this.

f) My usual comment about module name capitalization applies. Only capitalize "Block" in "the Block module" and leave lower-case otherwise.

g) NO CODE CHANGES. There is still at least one in the patch and this is a docs cleanup.

h) BlockTest.php

/**
-   * Test block visibility when using "pages" restriction but leaving
-   * "pages" textarea empty
+   * Tests block visibility when using "pages" restriction but leaving
+   * "pages" textarea empty.
    */
   function testBlockVisibilityListedEmpty() {

Needs one-line summary.

dcam’s picture

Status: Needs work » Needs review
FileSize
11.23 KB

Rerolled #14. This does not include any changes listed in #15.

jhodgdon’s picture

Status: Needs review » Needs work

status per #15

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!