Clean up API docs for the tracker module

Mostly looks good. Few tweaks needed :)

+++ b/modules/tracker/tracker.install
@@ -23,6 +28,9 @@ function tracker_enable() {
+ * @return array $schema
+ *   Returns the associative array with the schema information.

There's no need to document parameters or return for hook implementations -- those are found in the docs for the hook itself.

+++ b/modules/tracker/tracker.module
@@ -70,6 +76,9 @@ function tracker_menu() {
+ * Update tracking information for any items still to be tracked, as
+ * kept track by variable_get('tracker_index_nid').

On the other hand, this is good, as it explains what is being done by the hook implementation (and in the case of hook_cron, everyone will be different).

+++ b/modules/tracker/tracker.module
@@ -156,7 +171,13 @@ function _tracker_myrecent_access($account) {
 function _tracker_user_access($account) {

Missing a @param for this.

+++ b/modules/tracker/tracker.pages.inc
@@ -7,7 +7,15 @@
+ * Prints a listing of active nodes on the site. This is a menu callback.

I would keep this as one sentence. The format 'Page callback for ....' is fairly standard in core -- but maybe that's what you're trying to improve as part of this sprint?

Also, technically, it doesn't print ;

-18 days to next Drupal core point release.

joachim is right - our standard is NOT to put in param/return for hook implementations -- too much duplication -- and yes to put in specific information about this particular implementation (which you've done very well in this patch!) -- see standards at:
http://drupal.org/node/1354#hookimpl

Other things to note/fix:

a) Our Drupal overall writing style standards require a comma before and/or in a list:
Install, update and uninstall ==> Install, update, and uninstall

b) This needs some kind of rewrite - reads very awkwardly:

+ * Update tracking information for any items still to be tracked, as
+ * kept track by variable_get('tracker_index_nid').

c) As I noted on the other review recently, don't abbreviate words like "info", "var", and "max" in docs -- use "information", "variable", and "maximum".

d) As noted on the other issue:

* Prints a listing of active nodes on the site. This is a menu callback.

Having two sentences isn't really OK. How about:

Menu callback: prints a listing of active nodes on the site.

Or you could just do
Prints a listing of active nodes on the site.
(blank line)
This is the menu callback for path abc/foo.

e) Verb tense:

+  /**
+   * Set up the test class; initialize data needed for testing.

should be Sets

Otherwise, looks pretty good!

> then I'd suggest that a new issue be created so that the documentation can be made more explicit about @param, @return, and hook implementations.

Agreed. Go ahead and create it :)

The relevant passage from the hook implementation standards (at the top):

If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

Then farther down it says:

Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

I'll add a note there right now stating that you should never bother to do the param/returns.

This is added to the docs. I don't think we need a separate issue, since the standard was there (and being followed), just not written clearly enough. :)

I think we're close to adopting a standard there, so I'll review then...

You adopted the standard we adopted: http://drupal.org/node/1354#menu-callback hooray! Reviewing this patch... Actually, it looks like most of the review items I posted at http://drupal.org/node/1313510#comment-5157242 apply here, or at least some of them do, so I'll just refer to that and wait for a new patch.

Thanks!

Oh, one other nitpicky thing I see: Just put one space after the "." at the end of a sentence, not two. :)

Hi @aenw. I read through #15 and this is what I noticed. Looks like you fixed quite a few mistakes in the existing text!

+++ b/modules/tracker/tracker.moduleundefined
@@ -70,6 +70,11 @@ function tracker_menu() {
+ * Update tracking information for any items still to be tracked.  The
+ * variable 'tracker_index_nid' is 0 if there is nothing to be done here,
+ * otherwise it is set to ((the last nid that was indexed) - 1)) and used
+ * to select the nodes to process in this function.

This is a bit of a comma splice; the second sentence should be broken into two sentences. There's also one too many closing parentheses.

I'd suggest switching the order of the clauses in this explanation for clarity. Maybe:

The variable 'tracker_index_nid' is set to ((the last nid that was indexed) - 1) and used to select the nodes to be processed. If there are no remaining nodes to process, 'tracker_index_nid' will be 0.

Also, maybe it should begin with "Updates" for consistency with the summaries.

+++ b/modules/tracker/tracker.moduleundefined
@@ -148,7 +153,17 @@ function tracker_cron() {
+ *   The account id to check

Missing a period. Also, this is a minor point, but I think it's more standard to capitalize "ID" when it's used in a sentence (since "id" is a different word; see: Freud). I should add I've definitely seen it both ways, even in the same core files!

+++ b/modules/tracker/tracker.moduleundefined
@@ -156,7 +171,17 @@ function _tracker_myrecent_access($account) {
+ *   The user account id to track.

Same note about "ID".

+++ b/modules/tracker/tracker.moduleundefined
@@ -164,6 +189,8 @@ function _tracker_user_access($account) {
+ * Add new tracking information for this node since it's new.

@@ -171,6 +198,8 @@ function tracker_node_insert($node, $arg = 0) {
+ * Add tracking information for this node since it's been updated.

@@ -178,6 +207,8 @@ function tracker_node_update($node, $arg = 0) {
+ * Delete tracking information for a node.

+++ b/modules/tracker/tracker.pages.incundefined
@@ -2,12 +2,21 @@
+ * Query the database for info, add RDFa info if applicable, and generate the
+ * render array that will be used to render the page.

Same suggestion as before; maybe using the same verb tense as the summaries (3rd person present indicative) would be more consistent.

+++ b/modules/tracker/tracker.pages.incundefined
@@ -2,12 +2,21 @@
+ * User page callbacks for tracker.module

Missing period.

+++ b/modules/tracker/tracker.pages.incundefined
@@ -2,12 +2,21 @@
+ * @return array
+ *   An array that can be rendered; it will be themed by a call to
+ *   page_theme().

Perhaps: "A renderable array to be themed in page_theme()."

+++ b/modules/tracker/tracker.testundefined
@@ -5,11 +5,31 @@
+
+  // @TODO this var doesn't seem to be used; should be removed?

This isn't quite the standard format for @todos; search for @todo on http://drupal.org/node/1354.

However, rather than adding the @todo at all here, let's open a followup issue.

+++ b/modules/tracker/tracker.testundefined
@@ -18,6 +38,11 @@ class TrackerTest extends DrupalWebTestCase {
+   *
+   * @see DrupalWebTestCase::setUp()

I don't think we need @see to the parent's methods; they are already listed automatically in the API documentation and are furthermore in the method summary.

+++ b/modules/tracker/tracker.testundefined
@@ -45,18 +70,18 @@ class TrackerTest extends DrupalWebTestCase {
-    $this->assertNoText($unpublished->title, t('Unpublished node do not show up in the tracker listing.'));
-    $this->assertText($published->title, t('Published node show up in the tracker listing.'));
+    $this->assertNoText($unpublished->title, t('Unpublished node does not show up in the tracker listing.'));
+    $this->assertText($published->title, t('Published node shows up in the tracker listing.'));
...
-    $this->assertNoText($published->title, t('Deleted node do not show up in the tracker listing.'));
+    $this->assertNoText($published->title, t('Deleted node does not show up in the tracker listing.'));

@@ -88,8 +113,8 @@ class TrackerTest extends DrupalWebTestCase {
-    $this->assertNoText($unpublished->title, t("Unpublished nodes do not show up in the users's tracker listing."));
-    $this->assertText($my_published->title, t("Published nodes show up in the user's tracker listing."));
+    $this->assertNoText($unpublished->title, t("Unpublished node does not show up in the users's tracker listing."));
+    $this->assertText($my_published->title, t("Published node shows up in the user's tracker listing."));

@@ -211,8 +236,8 @@ class TrackerTest extends DrupalWebTestCase {
-    $this->assertText('1 new', t('New comment is counted on the tracker listing pages.'));
-    $this->assertText('updated', t('Node is listed as updated'));
+    $this->assertText('1 new', t('1 new comment is counted on the tracker listing pages.'));
+    $this->assertText('updated', t('A node is listed as updated'));

@@ -222,11 +247,11 @@ class TrackerTest extends DrupalWebTestCase {
-    $this->assertText('1 new', t('New comment is counted on the tracker listing pages.'));
+    $this->assertText('1 new', t('1 new comment is counted on the tracker listing pages.'));

@@ -239,7 +264,7 @@ class TrackerTest extends DrupalWebTestCase {
-    $this->assertText($node->title, t('Node is displayed on the tracker listing pages.'));
+    $this->assertText($node->title, t('A node is displayed on the tracker listing pages.'));

@@ -249,6 +274,6 @@ class TrackerTest extends DrupalWebTestCase {
-    $this->assertText(t('No content available.'), t('Node is displayed on the tracker listing pages.'));
+    $this->assertText(t('No content available.'), t('A node is displayed on the tracker listing pages.'));

As a best practice, we should patch these lines in a separate issue, since they are not a part of the code doxygen. See: http://webchick.net/please-stop-eating-baby-kittens. It's especially important in this case because these are string changes.

+++ b/modules/tracker/tracker.testundefined
@@ -222,11 +247,11 @@ class TrackerTest extends DrupalWebTestCase {
+   * Tests that publish/unpublish works at admin/content/node

Missing period.

So, in summary, there are a couple things I'd recommend opening non-documentation followup issues for. You can link the followup issues here in this issue for reference.

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.

Per #16, @aenw created new issues for #1338110: remove the protected variable $new_node if it's not used in tracker.test and #1338152: Correct grammar for test messages in tracker.test . Issue updated for D8 /core,#16,#17.

After providing a patch on #1338152: Correct grammar for test messages in tracker.test , I posted #18 to avoid confusion since neither were linked at the time. I didn't realize this was one of the documentation api meta threads - by all means, ignore the patch on #18 and post your revision.

Hi, I hope there is no bad feelings on this! It's great that we have multiple people interested in documentation issues...

kathyh -- normally when an issue is assigned to someone, and they have been consistently active on the issue in the recent past, in the Drupal community we usually let that person take care of the patches for the issue. And if you want to take over an issue that appears to be abandoned, the usual way would be to post a comment saying "hey xyz, are you still working on this?" and if there is no response in a few days, then you can assign it to yourself and take it on.

Anyway... for the moment I'll assume we're starting from the patch in #18, and I hope kathyh and aenw can sort out who wants to continue with this issue.

The patch looks pretty good, but I see a couple of things that need to be fixed:

a)

+ * 'tracker_index_nid' is set to ((the last nid that was indexed) - 1) and used

In text, "nid" should be "node ID".

b)

 /**
- * Access callback for tracker/%user_uid_optional.
+ * Access callback: Determines access permission for a user's own account.
+ *
+ * Callback path: tracker/%user_uid_optional

Our standard calls for this last line to just say "Path:" not "callback path".
http://drupal.org/node/1354#menu-callback

c)

+ * @param int $account
+ *   The account ID to check.
+ * @return boolean
+ *   Return TRUE if a user is accessing tracking info for their own account

Needs blank line between param doc and @return. And we don't normally start an @return doc with "Return...". Just say what is returned.... although for an access callback we don't normally need @return at all (see http://drupal.org/node/1354#menu-callback).

d)
(b) and (c) apply to the next docblock too.

e)

 /**
- * Menu callback; prints a listing of active nodes on the site.
+ * Page callback: Generates a page of tracked nodes for the site.
+ *

Missing the path here. See standards page above. Also the @return isn't necessary.

f)

+  /**
+   * The main user for testing. Information should be correct for this user.

First lines of docblocks should be one line, one sentence, and follow:
http://drupal.org/node/1354#constants

g) missing docblock:

  protected $other_user;
+
   protected $new_node;

h)

+  /**
+   * Overrides DrupalWebTestCase::getInfo().
+   */
   public static function getInfo() {

Leave this out for now. Under controversy at the moment at
#338403: Use {@inheritdoc} on all class methods (including tests)
Same for setUp() methods.

Hi @aenw,

Thanks for taking this on. Are you still working on this issue? If not, we'll unassign it in a day or two so that someone else can give it a try. (Feel free to assign it back to yourself if you'd still like to work on it, as well.) Thanks!

Putting it back in the queue.

jhodgdon,

I think I fixed all of the issues you noted but I did have a question.

When you say @return isn't necessary do you mean that it should be removed or just as a note?

Also this is my first patch for core so let me know if I need to change anything.

Lets try again.

Thanks for picking up this issue!

Since I wrote the above comments, our standards for hook_menu() callbacks have changed (sorry!). The current standards are:
http://drupal.org/node/1354#menu-callback

So, here are some things to address in this patch:

a) Yes we want the return values now, so leave those in.

b) These types of lines now need to be removed:

+ *
+ * Path: tracker/%user_uid_optional

c) Duplicate lines:

+ *
+ * Deletes tracking information for a node.
+ *
+ * Deletes tracking information for a node.

d) Little punctuation nitpick:

+ * Updates indexing tables when a node is added, updated or commented on.

We follow the standard in Drupal docs that you need a comma here before "or".

e)

+ *   A renderable array to be themed in page_theme().

Just say "A renderable array" here. page_theme() doesn't exist anyway. :)

f) What does this "information should be correct" line mean? Maybe just omit it?

+  /**
+   * The main user for testing. 
+   *
+   * Information should be correct for this user.
+   *
+   * @var object
+   */

I took care of the things you listed but I had a couple more questions.

In the docs it says

For .install files, the following template is used:
/**
 * @file
 * Install, update and uninstall functions for the XXX module.

Should this have a comma after update following the standard comma use or not? I think the docs need to be updated but because that isn't necessarily a hard rule I can see how it would not be there also. I just want to make sure this is following the docs correctly.

I know this is meant to fix the documentation so I didn't change some of the long db_query and db_query_range statements from a single line to multiple lines. Should I do that or leave it as a single line? The example show it as multiple but I know it would wrap it anyways due to the display.
http://api.drupal.org/api/drupal/core%21includes%21database%21database.i...

A couple more minor changes.

RE #29 comma - yes, eek! Fixed in the standards doc. THANKS!

RE long lines - the 80-character length limit only applies to documentation blocks, not code.

I reviewed the latest patch... It goes just slightly out of the scope of the Official Docs Cleanup, but all the changes look good to me, and they are all either comments or whitespace, so I think we should just get it in. Thanks!

jhodgdon,

Thanks for all of your help along the way. Is there anything else I need to do to this or just wait and see if any other issues come up with it and then someone else will commit it?

That's it for you for now. After it gets (hopefully) committed to 8.x, you could come back and port the patch to 7.x.

Committed to 8.x. :)

This patch can be ported to 7.x. AFAIK, it will take a bit more work than a quick re-roll.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -148,7 +153,16 @@ function tracker_cron() {
- * Access callback for tracker/%user_uid_optional.
+ * Access callback: Determines access permission for a user's own account.
+ *
+ * @param int $account
+ *   The account ID to check.
+ *
+ * @return boolean
+ *   TRUE if a user is accessing tracking info for their own account and
+ *   has permission to access the content.
+ *
+ * @see tracker_menu()

@@ -156,7 +170,16 @@ function _tracker_myrecent_access($account) {
- * Access callback for user/%user/track.
+ * Access callback: Determines access permission for an account.
+ *
+ * @param int $account
+ *   The user account ID to track.
+ *
+ * @return boolean
+ *   TRUE if a user has permission to access the account for $account and
+ *   has permission to access the content.
+ *
+ * @see tracker_menu()
  */

+++ b/core/modules/tracker/tracker.pages.incundefined
@@ -2,12 +2,20 @@
- * Menu callback; prints a listing of active nodes on the site.
+ * Page callback: Generates a page of tracked nodes for the site.
+ *
+ * Queries the database for info, adds RDFa info if applicable, and generates
+ * the render array that will be used to render the page.
+ *
+ * @return array
+ *   A renderable array.
+ *
+ * @see tracker_menu()
  */

I think we just need to remove the @param, @return, and @see from these for the backport.

Hmm, I don't actually see this commit in D8?

Alright, the rest of the missing D8 commits have been fixed, but this one is still missing, so setting back to RTBC for D8. Patch is in #30.

Moving to Jennifer's queue to take a look at.

Actually, I'm pretty sure the patch in #30 has already been applied. Moving back to D7.

Yep, looks like it made its way in sometime in the interim, probably a merge.

This is another good novice backport. See #35 for changes to make for a D7 patch.

I'm not sure if this is right or not because I couldn't find the documentation for the D7 page callbacks but I took out the things listed in http://drupal.org/node/1315214#comment-5623430 with the exception that I changed Menu callback; to Page callback:

It seemed odd to have a ; and I assumed it should be a :

This looks okay for backport to me. I was a bit confused by the non-doxygen changes but it looks like those were also in the patch for 8.x Thanks!

Yeah sorry about that. It was the first one I did and I wasn't quite sure how the docblocks worked.

Thanks! Committed to 7.x.