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.
Comment | File | Size | Author |
---|---|---|---|
#22 | aggregator-docs-1807556-22.patch | 24.41 KB | Albert Volkman |
#22 | interdiff.txt | 3.51 KB | Albert Volkman |
#19 | aggregator-docs-1807556-19.patch | 23.52 KB | Albert Volkman |
#19 | interdiff.txt | 1.47 KB | Albert Volkman |
#17 | aggregator-docs-1807556-17.patch | 23.52 KB | Albert Volkman |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedHere 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.
Comment #3
jhodgdonIt 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).
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @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.
Comment #5
Lars Toomre CreditAttribution: Lars Toomre commentedI am moving this to unassigned in the event that anyone else wants to take this further.
Comment #6
jhodgdonWell, I'll probably get to reviewing it eventually. :)
Comment #7
Lars Toomre CreditAttribution: Lars Toomre commentedFeel 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.
Comment #8
jhodgdonI 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():
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:
If you want to be obsessive about the verb, move it to after "content".
d)
Grammatically, this should be "the feed" not "a feed". Similar for other similar docs additions.
e)
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)
Capitalize Aggregator module. [applies to other CSS files too, and various test classes]
g)
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)
OPML not Opml (acronym)
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedThanks 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.
Comment #10
Lars Toomre CreditAttribution: Lars Toomre commentedHere 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.
Comment #11
jhodgdonRegarding #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!
Comment #12
Lars Toomre CreditAttribution: Lars Toomre commentedAttached is a self-review for when this patch is next re-rolled.
Not sure if this should be a capitalized 'Aggregator' or not.
Should this be 'Aggregator' too?
Ibid.
Ibid.
'plural-formatted as' reads better.
Wonder if this should be 'list all of the aggregated items.' instead? Same in next several doc blocks.
Should this be 'HTML-formatted'?
Probably should be 'the Aggregator module.' Same in one line docblock headers below.
Maybe 'Aggregator items of'?
Remove the extra blank line at the bottom.
Should this be 'Aggregator feed'?
Ibid.
'values are set to the category names.' reads much better.
'Tests the categorize feed' reads better.
Comment #13
Lars Toomre CreditAttribution: Lars Toomre commentedThe 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.
Comment #14
jhodgdonThanks 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:
d) aggregator.pages.inc
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
That @return is inconsistent with the first line, which says it's OMPL not HTML.
f) AggregatorTestBase.php:
If you are making another patch anyway, you could fix:
- if -> whether
- is -> are
- url -> URL
g) ImportOpmlTest.php
Needs comma before "and".
h) same file
Technically, OPML files are not feeds - should say "of an OPML file" probably?
i) RemoveFeedTest.php
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.
Comment #15
Albert Volkman CreditAttribution: Albert Volkman commentedFixed up.
Comment #16
jhodgdonCommitted to 8.x, and ready for backport. Thanks!
Comment #17
Albert Volkman CreditAttribution: Albert Volkman commentedHere we go for D7.
Comment #18
jhodgdonThanks! 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:
Should be NULL, not null in text.
b) This little bit has an indentation problem (aggregator.pages.inc):
and so does the next hunk in the patch:
c) aggregator.pages.inc
Not following standards at http://drupal.org/node/1354#menu-callback -- and the next several functions are not either. They should say "Page callback: ...".
Comment #19
Albert Volkman CreditAttribution: Albert Volkman commentedVerified 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?
Comment #20
jhodgdonRegarding 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.
Comment #21
jhodgdonComment #22
Albert Volkman CreditAttribution: Albert Volkman commentedMenu callback docs fixed.
Comment #23
jhodgdonLooks good -- I'll get this one committed soon. Thanks for all of your patching efforts!
Comment #24
jhodgdonCommitted to 7.x. Thanks again!
Comment #25
jhodgdon