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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre’s picture

Assigned: Unassigned » Lars Toomre

I am opening a new issue for the Aggregator module because there is a missing @param 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. A quick review indicates that there are missing docblocks in the tests, verb tense problems in the tests and missing @param directives in the main files.

Lars Toomre’s picture

Title: Clean up API docs for Aggregator module » Further clean up of API docs for Aggregator module
Status: Active » Needs review
FileSize
32.44 KB

Here is a patch (untested locally) that fixes more items in the documentation of Aggregator module in order to move it closer to compliance with general documentation and simpletest documentation standards.

jhodgdon’s picture

It would be great if you could recruit someone to review this, since there's a lot of additional documentation added in this patch (it's not just a verb tense cleanup).

Lars Toomre’s picture

Thanks @jhodgdon.

I am not sure if people really care about good documentation. I guess that missing @file directives do not really matter either.

At least, I have done the heavy lifting to get this closer to documentation standards. If anyone cares, they can review the patch in #2.

Lars Toomre’s picture

Assigned: Lars Toomre » Unassigned

I am moving this to unassigned in the event that anyone else wants to take this further.

jhodgdon’s picture

Well, I'll probably get to reviewing it eventually. :)

Lars Toomre’s picture

Feel free to review it whenever @jhodgdon. It really should not fall to you and that was my point in #4.

From a developer experience perspective, I would hope that you would spend your limited volunteer time reviewing the various type hinting patches that add missing type hints to the hook functions in the *.api.php files. Those would really improve what a developer sees documentation-wise on the api.drupal website.

Reviewing a patch like this one, from my perspective, is far less useful because all it does is bring consistency to our documentation. For much of the last year, no one apparently has even cared about what was missing in the #2 patch. It is sad, but that appears to be the truth.

jhodgdon’s picture

Status: Needs review » Needs work

I took a look at this patch today. Most of the changes are needed clean-ups, thanks! A few things need attention.

a) Many of the changes at the top of this patch, I would say, don't do much to change compliance with standards. We don't have a standard that says "all @see lines should be alphabetic" (that seems to be something you wish were a standard, but it isn't), and I think some of your changes make those sets of lines less logical. Also, we don't have a clear standard that says we always need a blank line between @ingroup and @see sections, or for the order of those sections, as far as I know. Let's stick to changes that actually improve standards compliance. Thanks!

b) I am not sure about this change to hook_aggregator_process_info():

* If this hook is not implemented aggregator will use your module's file name
  * as title and there will be no description.
  *
+ * @param object $feed
+ *   The feed object that the title and short description of the processor
+ *   describes.
+ *
  * @return
  *   An associative array defining a title and a description string.
  *

As far as I can tell, $feed is never actually passed into this hook when it is invoked, so I am not sure where the idea for this documentation came from... I think we should probably remove that parameter from the hook docs, but that's obviously a separate issue:
#1811218: hook_aggregator_process_info() - remove $feed from docs
which I just filed. So let's leave this change out until that other issue is decided.

c) This change is not grammatical:

 /**
- * Safely renders HTML content, as allowed.
+ * Renders safely HTML content, as allowed.

If you want to be obsessive about the verb, move it to after "content".

d)

+ * @return string
+ *   The rendered list of items for a feed.
+ *
  * @see aggregator_menu()

Grammatically, this should be "the feed" not "a feed". Similar for other similar docs additions.

e)

* Callback function used by xml_parse() within aggregator_parse_feed().
+ *
+ * @param mixed $parser
+ *   The variable is not used in this function, but is included for consistency.
+ * @param string $name
+ *   The name of the opening tag.
+ * @param array $attributes
+ *   The link attributes.
  */
 function aggregator_element_start($parser, $name, $attributes) {

We don't want to document the parameters or return value for callback functions -- it's enough to say it's a callback for a standard PHP function like xml_parse(), which documents what the callbacks should look like.

f)

+/**
+ * Right-to-Left styles for theme in aggregator module.
+ */
+

Capitalize Aggregator module. [applies to other CSS files too, and various test classes]

g)

- * Uses drupal_http_request() to download the feed.
+ * This class uses drupal_http_request() to download the feed.

This change is totally unnecessary. We use this documentation style a lot in Drupal docs -- it's kind of a continuation of the first line, and the implied subject of "This function..." still applies. We don't need the extra words.

h)

+  /**
+   * Tests the import of an Opml feed.
+   */
   function testOpmlImport() {

OPML not Opml (acronym)

Lars Toomre’s picture

Thanks for the review @jhodgdon. I will try to get to a re-roll of this later when I have a chance.

Regarding a), I have been doing enough of these module reviews now to observe that our documentation standards are really inconsistent with regard to any @see and @ingroup directives. In existing code (untouched by me), sometimes it is @ingroup first and then @see's in whatever order; some it is a @see's for validate/submit and then a blank like for @ingroup. To me at least this has been confusing.

Hence, when I have focused on reviewing a docblock, I have made sure that there is the following:

a) A one-line summary.
b) An @params are grouped in function call order with no blank likes between.
c) If there is @return directive, it is separated by a blank line and followed by an explanation.
d) If there is @throws directive, make sure it is separated by a blank line.
e) If there are one or more @see directives, make sure they are grouped together with a blank line before and ordered in a logical order (which if there is more than validate/submit, implies alphabetical order).
f) If there is an @ingroup directive, make sure it is separated by a blank line.
g) If there is a @todo directive, make sure it is last and also separated by a blank line.

For readability and ease of understanding, I believe it makes sense to separate different types of @ directives with a blank like and to group @see directives in some logical order. If there are more than just validate/submit, I have been changing the order to alphabetical.

Similarly, if I had my way with our coding standards, I would address the mis-mash of @use statements at the top some core files. When starting to read some code files, there is no rhyme or reason to the order; in others, they are grouped by say Symphony and Drupal and alphabetical within those groupings.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
15.51 KB
32.99 KB

Here is an updated (and locally untested) patch from #2 that incorporates the other comments from #8. Also attached is an interdiff relative to #2. Thanks for your review @jhodgdon.

jhodgdon’s picture

Regarding #9 - you are making up your own standards in e-g.... The thing you don't seem to agree with, but has been a guiding philosophy in our coding and documentation standards is that we don't really have to have everything regimented. The real goal is to be able to read the documentation and find all the information a programmer would need. The exact order of the stuff at the bottom of the doc block has not been deemed important enough to spend the time to agree upon or enforce a standard for it, and I don't see a compelling reason to standardize it now. And so I also don't see a compelling reason to spend time making/reviewing patches that change documentation to conform to standards that you've invented... The other changes you are making are *very* much appreciated, but it would take me a lot less time to review/commit your patches if you could please confine your efforts to the actual standards.

Anyway, I'll see what I can do about reviewing this next one in the next few days. Thanks!

Lars Toomre’s picture

Status: Needs review » Needs work

Attached is a self-review for when this patch is next re-rolled.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -455,8 +458,9 @@ function aggregator_admin_refresh_feed($feed) {
 /**
  * Form constructor for the aggregator system settings.

Not sure if this should be a capitalized 'Aggregator' or not.

+++ b/core/modules/aggregator/aggregator.installundefined
@@ -259,7 +259,7 @@ function aggregator_schema() {
- * Moves aggregator settings from variables to config.
+ * Move aggregator settings from variables to config.

Should this be 'Aggregator' too?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -267,13 +267,13 @@ function aggregator_menu() {
- * Title callback: Returns a title for aggregatory category pages.
+ * Title callback: Returns a title for aggregator category pages.

Ibid.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -267,13 +267,13 @@ function aggregator_menu() {
- *   An aggregator category title.
+ *   A string with the aggregator category title.

Ibid.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -794,7 +794,7 @@ function aggregator_sanitize_configuration() {
- *   Plural-formatted "@count items"
+ *   A string that is plural-formatted "@count items"

'plural-formatted as' reads better.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -43,8 +49,12 @@ function aggregator_page_source($feed) {
  *   The feed for which to list all the aggregated items.

Wonder if this should be 'list all of the aggregated items.' instead? Same in next several doc blocks.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -347,6 +370,9 @@ function template_preprocess_aggregator_item(&$variables) {
+ * @return string
+ *   An HTML formatted string.

Should this be 'HTML-formatted'?

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -374,6 +400,9 @@ function aggregator_page_sources() {
 /**
  * Page callback: Displays all the categories used by the aggregator.

Probably should be 'the Aggregator module.' Same in one line docblock headers below.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -399,6 +428,9 @@ function aggregator_page_categories() {
 /**
  * Page callback: Generates an RSS 0.92 feed of aggregator items or categories.

Maybe 'Aggregator items of'?

+++ b/core/modules/aggregator/aggregator.parser.incundefined
@@ -242,6 +242,7 @@ function aggregator_element_end($parser, $name) {
  * Callback function used by xml_parse() within aggregator_parse_feed().
+ *

Remove the extra blank line at the bottom.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.phpundefined
@@ -34,10 +34,15 @@ function setUp() {
-   * Create an aggregator feed (simulate form submission on admin/config/services/aggregator/add/feed).
+   * Creates an aggregator feed.

Should this be 'Aggregator feed'?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.phpundefined
@@ -54,7 +59,7 @@ function createFeed($feed_url = NULL) {
-   * Delete an aggregator feed.
+   * Deletes an aggregator feed.

Ibid.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.phpundefined
@@ -175,7 +185,11 @@ function getFeedCategories($feed) {
+   * @return array
+   *   An associative array keyed by category ID and values of the category

'values are set to the category names.' reads much better.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/CategorizeFeedTest.phpundefined
@@ -7,7 +7,11 @@
+/**
+ * Tests categorize feed functionality in Aggregator module.
+ */
 class CategorizeFeedTest extends AggregatorTestBase {

'Tests the categorize feed' reads better.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
30.31 KB

The attached patch addresses the items in #12 as well as removing all of the blank lines added at beginning or end of Test class declarations.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the patch! Looks better. A few things still need fixing:

a) The word "Aggregator" should only be capitalized if it is being used as the proper name of the module though -- only if it says "the Aggregator module". So several of the changes that capitalized it are inappropriate, such as when it says "the aggregator system" or "the aggregator administration page".

b) Take out the changes that alphabetize @see lines. They were mostly in a better logical order before and we don't have a standard that they should be alphabetic.

c) This line from aggregator.module needs a . at the end:

- *   Plural-formatted "@count items"
+ *   A string that is plural-formatted as "@count items"
  */
 function _aggregator_items($count) {

d) aggregator.pages.inc

/**
- * Prints the RSS page for a feed.
+ * Prints the RSS page for the feed.

Take this change out. In the first line of a function, there is no referent to make "the" make any sense (which specific feed would it refer to?).

e) aggregator.pages.inc

* Page callback: Generates an OPML representation of all feeds.
  *
  * @param $cid
- *   If set, feeds are exported only from a category with this ID. Otherwise,
- *   all feeds are exported.
+ *   (optional) If set, feeds are exported only from a category with this ID.
+ *   Otherwise, all feeds are exported. Defaults to NULL.
+ *
+ * @return string
+ *   An HTML formatted string.

That @return is inconsistent with the first line, which says it's OMPL not HTML.

f) AggregatorTestBase.php:

/**
-   * Check if the feed name and url is unique.
+   * Checks if the feed name and url is unique.

If you are making another patch anyway, you could fix:
- if -> whether
- is -> are
- url -> URL

g) ImportOpmlTest.php

/**
-   * Submit form with invalid, empty and valid OPML files.
+   * Submits form with invalid, empty and valid OPML files.

Needs comma before "and".

h) same file

+  /**
+   * Tests the import of an OPML feed.
+   */
   function testOpmlImport() {

Technically, OPML files are not feeds - should say "of an OPML file" probably?

i) RemoveFeedTest.php

+/**
+ * Test the removal of a feed functionality in the Aggregator module.
+ */
 class RemoveFeedTest extends AggregatorTestBase {

Verb tense is wrong.. The wording is a bit awkward on several of these added description lines for classes too... maybe "Tests functionality for removing feeds in the Aggreator module." would be easier to read? Similar for the other classes you added description lines to.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
15.65 KB
26.7 KB

Fixed up.

jhodgdon’s picture

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

Committed to 8.x, and ready for backport. Thanks!

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
23.52 KB

Here we go for D7.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! There are a few problems with this patch, but it mostly looks good... We should verify these problems are not also in the 8.x code base:

a) aggregator.admin.inc:

+ *   (optional) If editing a feed, the feed to edit as a PHP stdClass value; if
+ *   adding a new feed, null. Defaults to null.

Should be NULL, not null in text.

b) This little bit has an indentation problem (aggregator.pages.inc):

  * @return
-*   The rendered list of items for a category.
+*   The rendered list of items for the feed.
  */
 function aggregator_page_category($category) {

and so does the next hunk in the patch:

*
  * @return
-*   The rendered list of items for a category.
+*   The rendered list of items for the feed.
  *
  * @see aggregator_page_category()
  */

c) aggregator.pages.inc

/**
- * Menu callback; displays all the categories used by the aggregator.
+ * Menu callback; Displays all the categories used by the Aggregator module.
+ *
+ * @return string
+ *   An HTML formatted string.
+ *
+ * @see aggregator_menu()
  */
 function aggregator_page_categories() {

Not following standards at http://drupal.org/node/1354#menu-callback -- and the next several functions are not either. They should say "Page callback: ...".

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
23.52 KB

Verified that D8 is free of these issues. Fixed the issues pointed out. However, the page callback notes... aren't those only applicable to D8? Or are we backporting that standard to D7?

jhodgdon’s picture

Regarding the page callback standard, the proposed "Menu callback; Displays all the categories used by the Aggregator module." is not any good at all (doesn't follow any old or new standards), so yes let's go ahead and use the new standard for these.

jhodgdon’s picture

Status: Needs review » Needs work
Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
3.51 KB
24.41 KB

Menu callback docs fixed.

jhodgdon’s picture

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

Looks good -- I'll get this one committed soon. Thanks for all of your patching efforts!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x. Thanks again!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

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