Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#9 | drupal-clean_contact_docs-1811628-9.patch | 983 bytes | mbrett5062 |
#7 | drupal-clean_contact_docs-1811628-7.patch | 5.54 KB | mbrett5062 |
#3 | 1811628-3-contact-docs.patch | 5.94 KB | Lars Toomre |
#1 | 1811628-1-contact-docs.patch | 7.99 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedHere is a locally untested patch that cleans up some Tests and other things in the Contact module from doing a full API docs review.
Comment #2
jhodgdonThis change doesn't make sense to do:
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():
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.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedThis 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.
Comment #4
jhodgdonLooks fine, thanks! I'll get it committed shortly.
Comment #5
jhodgdonThanks! This is committed to 8.x. I suppose it ought to be backported?
Comment #6
mbrett5062 CreditAttribution: mbrett5062 commentedWorking on backport now.
Comment #7
mbrett5062 CreditAttribution: mbrett5062 commentedHere is the 7.x patch.
Comment #8
jhodgdonOops. 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?
Comment #9
mbrett5062 CreditAttribution: mbrett5062 commentedFixed #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.
Comment #10
jhodgdonThanks! That patch is OK to go in, but I'm going to wait until next week to commit it (big sprint going on).
Comment #11
mbrett5062 CreditAttribution: mbrett5062 commentedGreat, 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.
Comment #12
jhodgdonCommitted to 8.x. I don't think we need to backport it to 7.x.