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 #1323720: Clean up API docs for block module.
This issue is focused on further changes to bring Block 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. There also appears to be at least one missing @file directive.
Comment | File | Size | Author |
---|---|---|---|
#16 | 1807652-16-block-docs.patch | 11.23 KB | dcam |
#14 | 1807652-14-block-docs.patch | 20.03 KB | Lars Toomre |
#11 | 1807652-11-block-docs.patch | 20.04 KB | Lars Toomre |
#9 | 1807652-9-block-docs.patch | 20.04 KB | Lars Toomre |
#3 | 1807652-3-block-docs.patch | 4.46 KB | Lars Toomre |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedAttached is just a start of a full review of the Block module for compliance with the documentation coding standards.
Comment #2
Lars Toomre CreditAttribution: Lars Toomre commentedIf anyone is interested in taking this full review further, please feel free to do so.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedAdded changes to block.api.php file to the above patch.
Comment #4
Lars Toomre CreditAttribution: Lars Toomre commentedHmmm... For some reason #3 was not an addition but completely separate.
Comment #5
jhodgdonThanks for the patches!
The block.api.php changes in the patch on #3 are incorrect -- see
http://drupal.org/node/1354#hooks
The patch in #1 looks pretty good. A couple of spots could use some attention:
a)
The doc that was there was more compliant -- see http://drupal.org/node/1354#callbacks
b) Why take out useful documentation? It should probably be on @param $variables and reworded, but it's still useful:
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @jhogdon for the comment about the patch #3.
Until your comment, I did not know that the one line directives in *.api.php files were different from normal file docblocks. Be aware that I probably have made the same error in other full review patches that I recently have rolled.
Comment #7
jhodgdonYeah, we have a lot of standards! If you think you've made the same error in other patches, and you know which patches, it would be very helpful if you could fix them... just a thought. :) I'll try to keep a look-out though.
Comment #8
Lars Toomre CreditAttribution: Lars Toomre commented@jhodgdon Thanks. Working with Drupal core commit process is a continual process of learning...
Based on what I remember, there might well be other recent patches that include:
- a) Verb tense fixes in *.api.php files,
- b) Verb tense corrections in *.install files,
- c) The phrase '???' when I knew that a proper text string was required,
- d) Something else ...
As I review the approximately 40 patches that I have created, but have not yet been committed, I will review them for the above criteria.
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedThe attached patch builds upon that in #1 and includes the comments from #5. It further includes other changes noted in a full review of all files in the block module, including its Test classes.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedHelps if there is not a stray character in block.admin.css... Let's see how this locally untested patch does.
Comment #12
Lars Toomre CreditAttribution: Lars Toomre commentedComment #14
Lars Toomre CreditAttribution: Lars Toomre commentedGrr... I am having editor problems. Let's try that one more time.
Comment #15
jhodgdonThanks for the patch!
A few things to clean up:
a) book.admin.inc:
Lots of extra spaces in this line.
b) My usual comment about not changing the order of @see statements applies. We don't have a standard that they should be alphabetical so it does not bring documentation into better standards compliance to alphabetize them, it just makes more work for patch makers and patch reviewers. Please remove those changes.
c) This type of change is also not necessary:
We don't need "this function..." ever to be in function docs really.
d) in block.module - the hook_menu access callbacks by convention should not have @return docs.
e) This is not a helpful change: (block.module)
I actually think it was clearer without that extra "Defaults to NULL" statement, because it already said what was happening if nothing was provided, and this extra thing makes you think "now what is that all about?". Really, adding all the "defaults to xyz" things that you seem to add to every patch doesn't add a lot to the docs -- the function with its default args is right below (or right at the top of the page if someone is looking at it in api.drupal.org). I really don't see that we need to go through every function and make sure this information is added to the documentation -- it's a lot of work for very little value added, and we don't have a standard that says we need this.
f) My usual comment about module name capitalization applies. Only capitalize "Block" in "the Block module" and leave lower-case otherwise.
g) NO CODE CHANGES. There is still at least one in the patch and this is a docs cleanup.
h) BlockTest.php
Needs one-line summary.
Comment #16
dcam CreditAttribution: dcam commentedRerolled #14. This does not include any changes listed in #15.
Comment #17
jhodgdonstatus per #15
Comment #18
jhodgdonThese issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.
Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!