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

This issue is focused on further changes to bring Contact module closer to D8/D7 documentation standards. This issue, for instance, will ensure 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

Status: Active » Needs review
FileSize
7.99 KB

Here is a locally untested patch that cleans up some Tests and other things in the Contact module from doing a full API docs review.

jhodgdon’s picture

Status: Needs review » Needs work

This change doesn't make sense to do:

- *   Array describing the contact category to be deleted. See the documentation
+ *   Array describing the Contact category to be deleted. See the documentation

We want the word "contact" to be capitalized if it is being used as a proper noun -- the name of the "Contact module". In this context, it is not a proper noun and should be lower-case. This applies to several other similar changes in the patch too.

The rest of the patch looks good though, thanks! I'm just not sure about this one documentation addition in ContactSitewideTest::updateCategory():

+   * @param array $categories
+   *   An array of categories.

What specific array of categories is this? This documentation doesn't tell me what it will be used for or how it relates to the function's actions or the other parameters.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
5.94 KB

This needed to be re-rolled after another patch changed the method ContactSitewideTest::updateCategory(). Because of the external patch changing that and a few other things, I am unable to create a valid interdiff against #1.

Here is a locally untested patch that addresses the rest of the comments from #2. Fortunately, this patch is rather small and should be easy to review.

jhodgdon’s picture

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

Looks fine, thanks! I'll get it committed shortly.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks! This is committed to 8.x. I suppose it ought to be backported?

mbrett5062’s picture

Assigned: Unassigned » mbrett5062

Working on backport now.

mbrett5062’s picture

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

Here is the 7.x patch.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Oops. We need to go back to 8.x and fix a couple of things that I missed in the previous review/commit:

a) Access callbacks should not have @return documentation.

b) Always say TRUE to refer to a Boolean TRUE value, not "true" or "True".

These both would apply to _contact_personal_tab_access(), but if you do (a), then (b) doesn't matter any more... but they might apply to other functions in the patch?

mbrett5062’s picture

Status: Needs work » Needs review
FileSize
983 bytes

Fixed #8 for 8.x new patch attached.

No other occurrences found in patch of @return statements in access callbacks.

Also added an empty line between @see statements and @ingroup for readability.

jhodgdon’s picture

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

Thanks! That patch is OK to go in, but I'm going to wait until next week to commit it (big sprint going on).

mbrett5062’s picture

Great, will wait till committed to do the 7.x patch.

Sorry it took a few days, only just got the internet back after Sandy.

Good luck with the sprint.

jhodgdon’s picture

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

Committed to 8.x. I don't think we need to backport it to 7.x.

Status: Fixed » Closed (fixed)

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