Clean up API docs for help module

Thanks! By the way, normally I would suggest leaving an issue assigned to you until it has been committed, unless you don't want to continue working on it if there are things that need to be fixed.

Reviewing the patch...

a)

+/**
+ * Provide a formatted list of available help topics.
+ */
 function help_links_as_list() {

Verb at beginning is not the right tense. See http://drupal.org/node/1354#functions -- and I think this function probably has a return value, so it needs @return docs?

b)

-
+  
+  /**
+   * Provide info about this test.
+   */
   public static function getInfo() {

That first line - you added a space to the line, which isn't good. There should not be spaces at the ends of any lines. Try to configure your editor to either remove spaces at ends of lines automatically, or at least show you where there are spaces at ends of lines.

And note that this occurs elsewhere in the file as well.

c)

   /**
-   * Login users, create dblog events, and test dblog functionality through the admin and user interfaces.
+   * Login users, create dblog events, and test dblog functionality through the
+   * admin and user interfaces.
    */
   function testHelp() {

- Function and method docs need to start with one 80-character-max sentence line.
- Wrong verb tense. Should probably be" "Logs in users, creates..., and tests..."
- This problem is repeated lower down in the same file on other functions.

d)

+  /**
+   * Provide info about this test.
+   */
   public static function getInfo() {

This is an override of a base class method. Standards for documenting it:
http://drupal.org/node/1354#classes
Should just say "Overrides ..." . Same with the setUp() method below there.

I fixed the meta-issue. Someone else put that text in there saying to unassign the issue. I took it back out.

You're right about the getInfo(). That doesn't appear to be defined on any of the test case base classes. I guess it's sort of an implicit interface that everyone knows about but isn't stated anywhere. Interesting.

Anyway, back to the patch... Looks much better! A few things to fix:

a)

-  // Output pretty four-column list
+  // Outputs a pretty four-column list

See http://drupal.org/node/1354#inline -- we actually don't require verb tenses like this in in-line comments (and they're not at all common -- usually the in-line comments describe what is happening and/or why, so they use verb tenses like the original here). If you do want to fix that comment, you could add a "." at the end.

b)

+  /**
+   * Provides info about this test.
+   */

I like this comment, except that we try not to abbreviate words like "information" in documentation. Note: this appears twice in the file.

c)

+   * Enables modules and create users with specific permissions.

create -> creates
Note: this appears twice in the file.

d) You could fix up the @param formatting here:

   /**
-   * Verify the logged in user has the desired access to the various help nodes and the nodes display help.
+   * Verifies the logged in user has access to the various help nodes.
    *
    * @param integer $response HTTP response code.

The @return in the next function is also not formatted correctly.

e)

  /**
-   * Get list of enabled modules that implement hook_help().
+   * Gets list of enabled modules that implement hook_help().

Should be "Gets the list..."

Better! Only a couple of things to fix:

a)

-  // Output pretty four-column list
+  // Output a pretty four-column list

This still needs to end in .

b)

+   * @return 
+   *   array Enabled modules.

The data type array should be on the same line as @return. And maybe it would be clearer to say "A list of the enabled modules." rather than just "Enabled modules"?

c)

 class NoHelpTestCase extends DrupalWebTestCase {
   protected $big_user;
-
+  
+  /**
+   * Provides information about this test.
+   */

- Your editor seems to have added a space to the end of that first line after "protected $big_user".
- The $big_user member variable needs a documentation block.

d) I took a look at the existing files in the modules/help directory. help.admin.inc is not following the (new!) standards at http://drupal.org/node/1354#menu-callback and needs an update. Also the first class in help.test doesn't have a docblock for the class, and the second one needs a verb update.

e) The docs for hook_help() in help.api.php -- actually, hook *definitions* are an exception to the verb rule:
http://drupal.org/node/1354#hooks -- so it actually should be saying "Provide online user help" as it does before your patch.

This is getting *very* close! Just three little items to fix and we're good to go!

a) That seems like a good idea for the help_page() path... however we normally use % for wildcards in hook_menu() router paths, so I think that would be a better idea. I've put that in the specs, and we should update this patch.

b) Just a small clarification on the menu callback standard from http://drupal.org/node/1354#menu-callback

 /**
- * Menu callback; prints a page listing a glossary of Drupal terminology.
+ * Menu callback: prints a page listing a glossary of Drupal terminology.

- After the :, the next word should be capitalized.
- Rather than calling it a "menu callback", we want to be specific about the type of callback in hook_menu() -- page, title, access, or whatever. In this case, it's "Page callback:".

c)

+  /**
+   * The users who will be created (one admin and one anonymous).
+   */
   protected $big_user;
   protected $any_user;

Each member variable should have a separate doc header. The reason is that each one will appear separately on api.drupal.org, so each one needs its own description.

Not at all -- I hope you aren't either!

I think this is ready! Thanks for all the iterations.

Unless I missed a change somewhere we never add phpdic for getInfo() since it's just extending the base class which already has documentation.

See comment #4/5 above. Actually, getInfo() is not documented anywhere that we could find -- it is not on the base class apparently.

See #338403: Use {@inheritdoc} on all class methods (including tests) and http://drupal.org/node/325974. If you want to revert that decision then that needs a new issue with its own discussion.

OK, I'm reopening that issue, which I had never seen before.

Note that (in addition to the change suggested above) this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

Thanks @rc_100! A couple small things I noticed:

+++ b/core/modules/help/help.admin.incundefined
@@ -58,7 +75,7 @@ function help_links_as_list() {
-  // Output pretty four-column list
+  // Output a pretty four-column list.

It would be better not to include this change, since it's outside of the docblock and makes this patch more likely to collide with others.

+++ b/core/modules/help/help.testundefined
@@ -77,9 +93,10 @@ class HelpTestCase extends DrupalWebTestCase {
+   * @return array ¶

Trailing whitespace here.

And yeah I'm not sure which way to go on the test methods.

For the test methods, go ahead and leave those docblocks off. If we change this on that other issue, that issue will include a patch to put in the docblocks for all the core tests.

Setting to needs work as per #19. So if you can take care of that, and remove the getInfo() and setUp() docblocks from the tests, we can probably get this back to RTBC and get it committed. Thanks!

Everything in this patch looks good to me now. Let's get it in. Thanks!

Committed/pushed to 8.x, thanks!

Couple menu callback things removed.

This looks fine for d7. Thanks!

Committed and pushed to 7.x. Thanks!