Clean up API docs for Aggregator module

Good start, thanks! A few comments:

a) For functions that are both page/form, I suggest:
* Page callback: Form constructor for the ...
(i.e., a combination of the page callback and form constructor standards, since the function is both) -- and @see links to the submit/validate handlers if there area any.

Also, note that for page callbacks and form constructors, you don't need @return generally, unless it's doing something odd/special.

b)

+ *   If editing a feed, the feed to edit as a PHP stdClass value; if adding a
+ *   new feed, NULL

Needs to end in .

c)

 * Create an alternative fetcher for aggregator module.

Should either be "the Aggregator module" or "aggregator.module". The latter is somewhat better, as it makes a link on api.drupal.org

d)

 /**
- * Implement this hook to remove stored data if a feed is being deleted or a
- * feed's items are being removed.
+ * Remove stored data if a feed is being deleted or a feed's items are being
+ * removed.

First line of function description needs to be one 80-character line/sentence, followed by blank line. This occurs several times in your patch.

e)

+ * Goes through all fetchers, parsers and processors and checks whether they
+ * are available.
  * If one is missing resets to standard configuration.

Wrap next line along with the modified line.

f)

 /**
- * Helper function for teaser length choices.
+ * Callback for drupal_map_assoc() within
+ * aggregator_form_aggregator_admin_form_alter().
  */
 function _aggregator_characters($length) {

See http://drupal.org/node/1354#functions -- scroll down to the Callbacks sub-section.

Always, I think -- don't you agree that that would be the best way to meet both standards?

Hey @xenophyle; here's what I found reading through the patch. There are some things that are grammatical cleanups not specifically targeted by the sprint, and a few others that should be fixed in the patch for sure:

+++ b/modules/aggregator/aggregator.api.phpundefined
@@ -11,21 +11,20 @@
+ * A fetcher downloads feed data to a Drupal site. The fetcher is called at the
+ * first of the three aggregation stages: data is downloaded by the active
+ * fetcher, it is converted to a common format by the active parser and finally,
+ * it is passed to all active processors that manipulate or store the data.
...
- *   The $feed object that describes the resource to be downloaded.
- *   $feed->url contains the link to the feed. Download the data at the URL
- *   and expose it to other modules by attaching it to $feed->source_string.
...
+ *   contains the link to the feed. Download the data at the URL and expose it
+ *   to other modules by attaching it to $feed->source_string.

@@ -68,13 +66,12 @@ function hook_aggregator_fetch_info() {
- * A parser converts feed item data to a common format. The parser is called
- * at the second of the three aggregation stages: data is downloaded by the
- * active fetcher, it is converted to a common format by the active parser and
- * finally, it is passed to all active processors which manipulate or store the
- * data.
+ * A parser converts feed item data to a common format. The parser is called at
+ * the second of the three aggregation stages: data is downloaded by the active
+ * fetcher, it is converted to a common format by the active parser and finally,
+ * it is passed to all active processors which manipulate or store the data.

@@ -149,23 +145,22 @@ function hook_aggregator_parse_info() {
- * fetcher, it is converted to a common format by the active parser and
- * finally, it is passed to all active processors which manipulate or store the
- * data.
+ * fetcher, it is converted to a common format by the active parser and finally,
+ * it is passed to all active processors that manipulate or store the data.

While we're changing these lines, do you think we should correct the run-on sentences? Maybe:

...first of three aggregation stages: first, data is downloaded by the active fetcher; second, it is converted to a common format by the active parser; and finally, it is passed...

+++ b/modules/aggregator/aggregator.api.phpundefined
@@ -11,21 +11,20 @@
+ *   The $feed object that describes the resource to be downloaded.$feed->url

There's a missing space here after the period and before the variable name.

+++ b/modules/aggregator/aggregator.api.phpundefined
@@ -180,17 +175,16 @@ function hook_aggregator_process($feed) {
+ * Expose the title and a short description of your processor.

I think the verb tense needs to be fixed here ("Exposes") and we can replace the word "your" with an article ("the" or "a," depending on what you think is more appropriate in context).

+++ b/modules/aggregator/aggregator.api.phpundefined
@@ -207,8 +201,7 @@ function hook_aggregator_process_info($feed) {
+ * Remove stored feed data.

I think this needs to be "Removes."

+++ b/modules/aggregator/aggregator.moduleundefined
@@ -276,11 +281,13 @@ function _aggregator_category_title($category) {
+ * Access callback: Finds out whether there are any aggregator categories.

I'd say "Determines" rather than "Finds out" here.

+++ b/modules/aggregator/aggregator.moduleundefined
@@ -276,11 +281,13 @@ function _aggregator_category_title($category) {
+ *   TRUE if there is at least one category and the user has access to them,
+ *   FALSE otherwise.

I'd either say "or FALSE otherwise," or add a semicolon as you have elsewhere.

+++ b/modules/aggregator/aggregator.moduleundefined
@@ -574,6 +582,12 @@ function aggregator_remove($feed) {
+ * Gets the fetcher, parser and processors.
+ *
+ * @return
+ *   An array containing the fetcher, parser and processors.
+ */

I'd add commas after "parser" here for clarity.

+++ b/modules/aggregator/aggregator.moduleundefined
@@ -721,14 +738,13 @@ function aggregator_filter_xss($value) {
+ * Checks and sanitizes aggregator configuration.

I'd add an article (either "the aggregator configuration" or "an aggergator configuration," depending on what fits).

+++ b/modules/aggregator/aggregator.parser.incundefined
@@ -164,7 +165,8 @@ function aggregator_parse_feed(&$data, $feed) {
 /**
- * Callback function used by the XML parser.
+ * Callback function used by the XML parser function xml_set_element_handler()
+ * within aggregator_parse_feed().
  */

@@ -212,7 +214,8 @@ function aggregator_element_start($parser, $name, $attributes) {
 /**
- * Call-back function used by the XML parser.
+ * Callback function used by the XML parser function xml_set_element_handler()
+ * within aggregator_parse_feed().
  */

@@ -234,7 +237,8 @@ function aggregator_element_end($parser, $name) {
 /**
- * Callback function used by the XML parser.
+ * Callback function used by the XML parser function
+ * xml_set_character_data_handler() within aggregator_parse_feed().

Can we add one-line summaries for these that describe what the functions actually do? Then a blank line, and the callback information after that. Also, I think we can eliminate the words "the XML parser function" and just say "Callback for xml_set_element_handler() within aggregator_parse_feed()."

Edit: And maybe parameter documentation while we're at it, but that's not required.

+++ b/modules/aggregator/aggregator.parser.incundefined
@@ -271,8 +275,8 @@ function aggregator_element_data($parser, $data) {
-      // The sub-element is not supported. However, we must recognize
-      // it or its contents will end up in the item array.
+      // The sub-element is not supported. However, we must recognize it or its
+      // contents will end up in the item array.

+++ b/modules/aggregator/aggregator.processor.incundefined
@@ -22,9 +22,9 @@ function aggregator_aggregator_process($feed) {
-        // Save this item. Try to avoid duplicate entries as much as possible. If
-        // we find a duplicate entry, we resolve it and pass along its ID is such
-        // that we can update it if needed.
+        // Save this item. Try to avoid duplicate entries as much as possible.
+        // If we find a duplicate entry, we resolve it and pass along its ID is
+        // such that we can update it if needed.

Technically this is outside the scope of the issue since these lines are not inside a docblock. (Reference: http://webchick.net/please-stop-eating-baby-kittens).

I'm not sure if I've caught everything this time through, but thanks for your patience and for all your work already. :)

Note that 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.

I have a new patch with the changes, but I didn't change the verb tense for the hook documentation, since I think it's supposed to be the way it is, according to http://drupal.org/node/1354#hooks.

Sorry, I'm not sure what you're referring to here? Could you clarify which line you mean?

Note that there is a testbot issue at the moment, which is why the patch failed. We'll want to re-test it once that is fixed.

re: hooks -- xenophyle is correct. Hook definitions (functions hook_*) in the api.php file should follow the verb/doc standards at http://drupal.org/node/1354#hooks , not the generic function verb/doc standards.

And I think the testbot issue is fixed... will try a retest.

#9: aggregator-clean-up-documentation-1325116-2.patch queued for re-testing.

I'm doing upgrades on my main computer, so I had some spare time to read carefully through this entire patch. It mostly looks great! A few things to finish up/fix:

a) aggregator_admin_remove_feed() and its validate/submit handler functions should all be linked with @see. Well, technically our standards (http://drupal.org/node/1354#forms) are that the form generator has @see to validate/submit, and the validate/submit have @see pointing to each other (since the main function is in the one-line description, it's not necessary to have @see too). Actually, I think this is the only form/validate/submit set in the file that isn't done this way -- the others look good. :)

b)

+ * Removes all items from a feed and redirect to the overview page.

redirect -> redirects

c)

+ *   - cid: The category id.

id -> ID. "id" is a psychological term. "ID" is short for identifier. This is a major pet peeve of mine. :)

d)

  * @param $feed
- *   The $feed object that describes the resource to be downloaded.
- *   $feed->url contains the link to the feed. Download the data at the URL
- *   and expose it to other modules by attaching it to $feed->source_string.
+ *   The $feed object that describes the resource to be downloaded. $feed->url
+ *   contains the link to the feed. Download the data at the URL and expose it
+ *   to other modules by attaching it to $feed->source_string.

Not your fault, but in text, I wouldn't use "$feed" as an adjective in that first sentence. It should probably say "A feed object representing the resource to be downloaded.". Then using $feed in the rest makes sense, because it is referring (appropriately) to the actual $feed parameter. This also applies to some of the other hook docs in the same file. And maybe it shouldn't be fixed in this patch, but if you are touching that line anyway...

e) Usage/style nitpick:

+ * Expose the title and a short description of your parser.

"the title"... "a description" sort of bothers me. How about "Expose the title and short description..." Actually, maybe even "Specify the title and short description..."? What does being "exposed" mean? [This also occurs for the processor hook below.]

f)

+ * @param category

Missing $ on $category.

g)

 /**
- * Callback function used by the XML parser.
+ * Performs an action when an opening tag is encountered.
+ *
+ * Callback function used by xml_set_element_handler() within
+ * aggregator_parse_feed().
  */
 function aggregator_element_start($parser, $name, $attributes) {

Technically, while this callback is set up by the call to xml_set_element_handler(), it's actually used in xml_parse(). This also applies to the next two callbacks.

h)

 /**
  * Helper function for teaser length choices.

Needs verbification. :)

i) This is possibly better in another issue, but:

 /**
- * Expire feed items on $feed that are older than aggregator_clear.
+ * Expires feed items on $feed that are older than aggregator_clear.

- I don't think $feed should be in a one-line description. Those are shown in all kinds of listings on api.drupal.org, and there is no reference point for what $feed is.
- What is aggregator_clear? Oh, I guess it's a variable, but that is not clear when looking at this description.
How about simply:
"Expires items from a feed, depending on expiration settings."
and then explaining in a new line how that is decided (if that is even necessary)?

Whew! Getting close...

Looks good to me -- thanks!

#16: aggregator-clean-up-documentation-1325116-3.patch queued for re-testing.

Needs one hunk resolved on account of #1247982: API doc for hook_aggregator_parse is incorrect. Attached patch is a reroll that applies the remaining corrections to the updated documentation. The attached text file shows that change; there are no other changes.

Thanks, looks fine.

Sorry I didn't mean to but I just broke this again with #743234: /aggregator does not use pagination. Another re-roll please?

Only change here is that aggregator_load_feed_items() docs already got cleaned up, so we drop that hunk.

This chunk also changed between your previous patch and #23:

 /**
- * Menu callback; displays a form containing items aggregated in a category.
+ * Page callback: Form constructor to list items aggregated in a category.
+ *
+ * Path: aggregator/categories/%aggregator_category/categorize
  *
  * @param $category
  *   The category for which to list all the aggregated items.
  *
  * @return
-*   The rendered list of items for a category.
+ *   The rendered list of items for a category.
  *
+ * @see aggregator_menu()
  * @see aggregator_page_category()
+ * @ingroup forms
  */
 function aggregator_page_category_form($form, $form_state, $category) {

Can you put that back to what it was?

 /**
- * Menu callback; displays a form containing items aggregated in a category.
+ * Page callback: Form constructor to list items aggregated in a category.
+ *
+ * Path: aggregator/categories/%aggregator_category/categorize
  *
  * @param $category
  *   The category for which to list all the aggregated items.
  *
- * @return
-*   The rendered list of items for a category.
- *
+ * @see aggregator_menu()
  * @see aggregator_page_category()
+ * @ingroup forms
  */
 function aggregator_page_category_form($form, $form_state, $category) {
   return aggregator_page_category($category);
 }

Hm, fail. Attached re-removes the @return. I made a diff against the #20 file manually to make sure there weren't any other botched hunks, though I think that was the only change that was in the same hunk as aggregator_load_feed_items().

racing catch again... :)

Alright, committed and pushed to 8.x, thanks for the re-rolls!

Yep, this is correct. (We will probably need a similar followup for node.module and perhaps others.)

Ouch good point, let's do those other follow-ups soon. Committed/pushed to 8.x.

D7 backport without the menu callback standard, with helpful interdiff showing what I changed.

Thanks! I think this is all fine for D7. My only slight worry was this bit:

 /**
- * Title callback for aggregatory category pages.
+ * Title callback: Returns a title for aggregatory category pages.

Technically this is part of the menu standards, but it does also bring this doc block more in line with our older/d7 verb standards, so I would vote for letting it stand.

Yeah, I wavered about that as well, but since the words "Title callback" were already there, I figured the change was okay.

My feeling exactly. By the way that failed testing on "failed to create checkout database" -- looks like a bot glitch, so I'll hit retest.

#34: d7-aggregator.patch queued for re-testing.

Yeah, I think we can let that slide.

Committed and pushed to 7.x. Thanks!

I am reopening this issue because there is a missing directive in aggregator.api.php file. This was discovered while writing a patch in #1807160-1: Add missing type hinting to Aggregator module docblocks to add missing type hinting to the aggregator.api.php file.

I hope to write a patch for this issue shortly and will do a complete review of the module docs so there may be additional changes too.

@Lars Toomre, instead of reopening old issues, please file new ones. Thanks!

Thanks @xjm. @jhodgdon indicated that we should use the existing ones. I will open a new issue.

Existing ones if they're still open, sure, but this one was closed almost a year ago. :)

Also, in general, as a reviewer, I prefer new issues because it can get really confusing to track what's been committed and what hasn't. IMO reusing existing issues should only happen in the case that something needs to be reverted, or if a partial patch is committed as a stopgap but the issue isn't resolved.

Opened #1807556: Further clean up of API docs for Aggregator module as a continuation of cleaning up the Aggregator module to conform with http://drupal.org/node/1354.