Problem/Motivation
Many of the API documentation headers in the Drupal Core code base do not conform to our documentation standards (http://drupal.org/node/1354).
Proposed resolution
The Documentation Team is holding a "sprint" to bring the documentation closer to compliance. To participate:
- Choose an unclaimed module or directory from the Remaining Tasks section below.
- If there is not already an issue there [NOTE: they should already be there now!], file a new issue and assign it to yourself. Issue parameters (you can use this handy link, and then edit the title and assigned fields):
- Project: Drupal Core
- Version: 8.x-dev
- Component: Documentation
- Category: task
- Title: Clean up API docs for [module or directory]
- Description:
Part of meta-issue [#xxxxx]
- Assigned: Assign it to yourself
- Tags: docs-cleanup-2011, needs backport to D7, Novice - Edit this issue summary here, and add your issue and user name to the Remaining Tasks section below (or if it was an existing but unassigned issue, just put your user name in the "Assigned" column—and be sure to assign the issue to yourself also).
- As always, when editing an issue summary, you should add a comment to the issue saying what you edited, so that anyone following this issue will get an email. In this case, provide a link to the new issue you filed and state which module/directory you are claiming.
- Make a patch that fixes the following, in each file in your module/directory:
- The file should have a @file documentation block, with an 80-character max one-line summary - see http://drupal.org/node/1354#files
- Each function, method, constant, class, etc. in the file should have a documentation block, starting with a one-sentence, 80-character max, one-line summary.
- Functions/methods one-line summary should start with a verb in the right tense. See http://drupal.org/node/1354#functions, http://drupal.org/node/1354#hooks, and http://drupal.org/node/1354#menu-callback
- There should be a blank line between the @param and @return sections (if any) in documentation blocks.
- @param and @return statements should be formatted correctly (see http://drupal.org/node/1354#functions).
- @see and @ingroup lines should be at the end of the documentation block.
- Lines in docblocks should wrap as close to 80 characters as possible without going over.
- Remove all trailing spaces, e.g. with
perl -pi -e 's, *$,,' $(find core -regex '.*\.\(engine\|html\|inc\|info\|install\|js\|json\|module\|php\|script\|sh\|sql\|test\|txt\|xml\)')
Bonus things that probably need be fixed, but maybe not in this sprint except in small doses (because they make the patches harder to review by the reviewer and the committer -- the items above are all pretty easy to review, since they are fairly mechanical/obvious)... If you find one of these problems in a single function in a file, go ahead and fix it. If you find a lot, file a separate issue and don't include them in your patch.
- All function/method parameters should have @param documentation.
- All functions/methods with return values should have @return documentation.
- Data types on parameters and return values - this is DEFINITELY not part of this sprint. Someone may be organizing a sprint for doing this... for now, don't even file issues on it. Follow issue #711918: Documentation standard for @param and @return data types if interested.
Also, in general, please open separate issues for cleaning up things you notice outside of doxygen blocks or that aren't directly listed above, and refrain from including fixes for them in your patches. We want to reduce as much as possible the collision between these cleanups and other issues. (For a little background on why this is important, see: http://webchick.net/please-stop-eating-baby-kittens.) You can also list other issues you notice under the "Remaining tasks" header of your issue summary.
- Attach the patch to the issue you filed, and set its status to Needs Review. Leave it assigned to yourself, to indicate you will continue working on it if it needs work. Monitor the issue status until it is marked "fixed".
- Start over at the first step if desired!
- If you find that you cannot complete an issue you claimed:
- Add a comment to the issue and un-assign it
- If you have a partial patch, attach it to the issue
- Return here and edit the issue summary - remove your user name from the "Assigned" column to indicate that the issue is available (but leave the link to the issue there). As usual, when editing issue summaries, also add a comment to the issue.
Remaining tasks
The following modules and/or directories need to be done (see Proposed Resolution steps above). Add issue links and user names to the list as these are assigned. Use this format (after the directory/module name in the list): - username - [#NNNN]
, where NNNN is the issue's node ID for the issue you filed. (NOTE: When pasting this into a line below, do not include the "code" tags, or the issues won't turn into nice color-coded links.)
* Except database and filetransfer subdirectories (~9.5k lines)
** Except files subdirectory. We don't want to alter the files in the simpletest/files directory, since they are used in tests
We also need to:
- Figure out specifically which types of changes we're doing in these patches that should and should not be backported to Drupal 7 (see comments 63/64 below).
- Reopen the issues above currently marked "fixed", and mark them for backporting to Drupal 7 instead, with a comment from (a) in them explaining which parts are to be backported.
- Figure out which d7/8 docs standards are not part of Coder, and which can be automatically tested, and file issues in Coder to get them added to the appropriate versions of Coder.
Comment | File | Size | Author |
---|---|---|---|
#80 | remove-trailing-spaces-1310084-80.patch | 12.87 KB | pillarsdotnet |
#73 | drupal-remove-single-double-quotes-on-doc-lists.patch.txt | 114.33 KB | Alan D. |
#37 | parse.php_.txt | 2.12 KB | xjm |
#25 | parse.php_.txt | 2.03 KB | xjm |
Comments
Comment #1
jhodgdonI think we can get started on this now! If catch/Dries/webchick thinks we should postpone, please comment here and we'll postpone. Also, adding tag, which should go on all of the individual issues.
Comment #2
jhodgdonOh yeah, I think this would be a great thing for Novice contributors to join in! Good way to learn the standards and learn patching. I suggest any novices pick a module with just a few files in it, or you might be a bit overwhelmed.
Comment #2.0
jhodgdonComplete the description of what to do
Comment #3
sven.lauer CreditAttribution: sven.lauer commentedNice. I will partake ... though I wonder if we should try and mitigate conflicting patches, perhaps by adding a tag like "docs-cleanup-2011-MODULE_NAME" (or something) to doc issues as they get marked RTBC? This way, sprint-participants would have an easy way to keep track of any patches that might get committed while they are working on their patch?
Comment #4
jn2 CreditAttribution: jn2 commentedI plan to take on some of this as well. Might start with the node module, when I get a chance.
Comment #5
jhodgdonRE $3 - The patches for this issue will not conflict with each other -- each issue deals with different files.
When committed, they will force rerolls of many other patches that might be in review on those files, and not just doc issues. ( That is why this needs to be done now, not when there is a code freeze or critical problems being fixed. ) So I don't think this kind of tagging effort will really do much to help anything...
We don't normally do anything special to coordinate issues in that way, and we also have the abiltity, using git, to do a git pull and have it update files you are working on.
Comment #6
sven.lauer CreditAttribution: sven.lauer commentedOkay. Another thing I was wondering about: What if the module I picked does not need any fixes (e.g. I just had a quick look at the contextual module, and it looked pretty good on Standards compliance)---maybe generally, there should be another point in the list "Once the patch gets committed, ..." (maybe: edit the issue summary and put a
<del>
around your module?)---maybe the idea is that the maintainers do this step, but I suppose there still should be a canonical way to indicate that a module is fine as is (just set the related issue to "works as designed", or something?).Comment #7
jhodgdonIf you pick a module, create an issue, and it does not need fixes, just add a comment and mark the issue as "works as designed". The issue statuses will show up in the list on the summary automatically if they are put in there with the [#NNNNN] syntax.
Or if you never make an issue at all, you can just edit the issue summary here and say something like "Already up to standards" on that line, so that we have a record that this module is fine.
Comment #7.0
jhodgdonAdd items to not do in this sprint in large doses
Comment #7.1
aenw CreditAttribution: aenw commentedmarked the statistics module as taken
Comment #8
jn2 CreditAttribution: jn2 commentedMade the issue for node module: #1313980: Clean up API docs for node module and added to issue summary.
Comment #9
aenw CreditAttribution: aenw commentedMade an issue for the statistics module: #1313510: Clean up API docs for statistics module and noted that I took it in the list above.
Comment #10
aenw CreditAttribution: aenw commentedMade an issue for the tracker module: #1315214: Clean up API docs for the tracker module and noted that I took it in the list above.
Comment #10.0
aenw CreditAttribution: aenw commentedAdded jn2, node module
Comment #11
aenw CreditAttribution: aenw commentedMade an issue for the entity module: #1315340: Clean up API docs for entity.inc and noted that I took it in the list above.
Comment #11.0
aenw CreditAttribution: aenw commentedadding formatting to the issue links for the node and statistics module, and noted that I'm taking the tracker module.
Comment #11.1
aenw CreditAttribution: aenw commentedmarked the entity module as taken, issue #1315340
Comment #12
jhodgdonI just edited the issue summary to remove the CODE tags in the list - we don't want those there - we want to see the issues as links. :)
Comment #12.0
jhodgdonRemoved code tags so we could see issue links. :)
Comment #13
xjmI'm feeling ambitious so I took #1315886: Clean up API docs for includes directory, files starting with A-C.
Comment #14
jhodgdonWow, that is ambitious indeed! And by the way, the reason it was broken up that way is that webchick originally suggested that for large cleanup patches, we do one for the entire includes directory... I thought that was a bit too big, so I took the liberty of breaking it up just slightly. What we really wanted to avoid was having one patch/issue per file.
Comment #15
xjmAs part of this sprint can/should we correct
@param
declarations if they are missing(optional)
andDefaults to foo.
for parameters with provided defaults?What about general violation of the 80-character limit in docblocks?
Comment #16
jhodgdonoptiona/defaults: You can. It does make the patch a slight bit harder to review quickly.
80-character limits: definitely, I'll add that to the issue summary right now.
Comment #16.0
jhodgdonUpdated issue summary.
Comment #17
xjmAlright; since this is a pretty fat cleanup to begin with (
common.inc
baby!), I'll file such corrections as separate issues.Comment #18
jhodgdonJust filed:
#1315992: No standard for documenting menu callbacks
We may need to get that resolved before cleanup patches are done!
Comment #18.0
jhodgdonAdd note about line wrapping as an item to fix.
Comment #19
nicl CreditAttribution: nicl commentedI have taken:
#1316902: Clean up API docs for lib/Drupal/core/Database
While this directory is pretty massive (bigger than I realised in fact! :( )I suspect that the docs should be pretty good for this given that most of the code is new.
I'm on holiday this week anyway, so will have plenty of time, but I'll let people know if it's too much.
nicl
Comment #20
jhodgdonYou are rather over-optimistic. I don't think the classes and interfaces in there have been all that well documented. However, it may be too much to clean up... if it becomes too overwhelming, please come back and do a different module/file that is more tractable.
Comment #20.0
jhodgdonHave created an issue for includes/database so am adding this information to the summary along with issue link.
Comment #20.1
xjmUpdated issue summary.
Comment #20.2
xjmUpdated issue summary.
Comment #21
xjmSince the patch for #1315886: Clean up API docs for includes directory, files starting with A-C was already nearly 4000 lines long by the time I got done with
common.inc
, I updated the summary to split the includes directory into four hunks based on the total number of lines in the files.Comment #22
xjmAw hell, I might as well take the whole includes directory since I now have a process with emacs macros and everything. I can always throw the issues back if I get sick of this. :)
Comment #22.0
xjmUpdated issue summary.
Comment #23
jhodgdonI wonder if your emacs macros could be turned into some scripts that others could use to automate some of the process?
Comment #24
xjmHmm, well, they're not that smart and require user interaction. However, the following parts of the cleanup are hypothetically scriptable:
*
are under 80 characters.*
.*/
, all lines include @ingroup or @see.Coder presumably throws an error if a function or method is missing a docblock. I wonder if we could peel out that check without sending the whole of core through all coder's tests?
I could write a PHP or shell script that prints a list of notices for these things, but there wouldn't be any user interaction.
Comment #25
xjmparse.php
.drush scr parse.php
.It'll print a bunch of stuff to your terminal telling you which lines violate the above parts of the cleanup. Not the greatest UX, but at least you can use it to check for anything you've missed.
Oh, the line numbers are in programmer, so add one if your editor doesn't start with line 0. Should be obvious. ;)
NB: Code is not guaranteed to not be a poorly-factored baby-eating atrocity.
Comment #25.0
xjmUpdated issue summary.
Comment #26
xjmOh, when I ran #25, it reminded me of a question I had: Should we enforce the 80 character limit inside
@code
examples, or ignore them when they go over?Comment #27
jhodgdonThe 80-character limit can be ignored in @code areas.
Comment #27.0
jhodgdonUpdated issue summary.
Comment #27.1
jhodgdonUpdate to include standard on menu callbacks, and also not backporting to Drupal 7
Comment #27.2
rc_100 CreditAttribution: rc_100 commentedMade the issue for help module: #1319904: Clean up API docs for help module and added to the issue summary.
Comment #27.3
jhodgdonRemove idea of unassigning when you attach a patch
Comment #28
xjmRegarding #25, note that it does
strlen($line)
, which counts the terminal linebreak as a character. So, lines that are 81 chars by the script's count may or may not wrap, depending on your editor. I've asked for clarification for this in #1315886-25: Clean up API docs for includes directory, files starting with A-C; see comments #23, #25, and #30.Comment #29
jhodgdonDo the 81-character lines (by your count) wrap in your editor? They don't in mine (emacs). Would like some other opinions...
Comment #30
xjm#29: Yes, the 81-char lines wrap for me, by one adorable little errant character. I develop in 80-col terminal windows. If I toggle truncate lines, the final letter (but not the end of the line) fits within the window.
If this doesn't bother anyone else, I'll change the script to strip off
\n
before it checks the line length, and cope with the errant letters. :) I guess it would just be good to know one way or the other whether those lines count.Comment #31
jhodgdonThat 80-character standard has been around for ages, and I'm not sure whether it was meant to include the \n or not really. Maybe ask in IRC for other opinions?
Comment #32
xjmTried coder, but it apparently doesn't check character count.
Comment #33
webchickCan we get follow-up issues in the Coder queue for anything that can be automated and currently isn't? It always makes me die inside when I see humans doing any work that robots can be doing. Let's get the checks in so that when Coder and Testbot are integrated together we don't introduce anymore regressions.
Comment #34
jhodgdonThe only things I think Coder Review could check are 80-character wrapping in doc lines, and blank between param/return. And Coder Upgrade could probably fix the latter. So, yeah, let's file issues if there aren't already.
Comment #35
jhodgdonI found this existing issue on inline comments:
#150626: Check for proper inline comment formatting
And this one on docblocks:
#821910: PHPDoc rules (and not Doxygen as one would assume)
Comment #36
jhodgdonThis one too:
#521452: Update to check for new function header doc standards
So I think the issues have been filed.
Comment #37
xjmDon't use #25. Attached script corrected to follow our "de facto standard" on line length (stripping the newline character before checking a line's length). ;)
Comment #37.0
xjmAdd to do item to format param/return statements
Comment #38
xenophyle CreditAttribution: xenophyle commentedMade an issue for the block module: #1323720: Clean up API docs for block module and noted that I took it in the list above.
Comment #38.0
xenophyle CreditAttribution: xenophyle commentedassigned block module to myself
Comment #39
xenophyle CreditAttribution: xenophyle commentedMade an issue for the aggregator module: #1325116: Clean up API docs for Aggregator module and noted that I took it in the list above.
Comment #40
xjmEdit: Never mind; I was mistaken. Click on the "core" tab to review core includes in coder.
Comment #40.0
xjmassigning aggregator module to myself
Comment #40.1
jn2 CreditAttribution: jn2 commentedNote about issue status
Comment #40.2
jn2 CreditAttribution: jn2 commentedStatus note no longer needed.
Comment #40.3
Lars Toomre CreditAttribution: Lars Toomre commentedAssigning clean up of dblog module to Lars Toomre
Comment #40.4
Lars Toomre CreditAttribution: Lars Toomre commentedAssign the clean up of image module to Lars Toomre.
Comment #40.5
Lars Toomre CreditAttribution: Lars Toomre commentedAssign clean up of locale module to Lars Toomre.
Comment #40.6
Lars Toomre CreditAttribution: Lars Toomre commentedAdded issue for clean up of the field module.
Comment #40.7
Lars Toomre CreditAttribution: Lars Toomre commentedAdded issue reference for clean up of field_ui module.
Comment #40.8
Lars Toomre CreditAttribution: Lars Toomre commentedDocumented creation of issue for clean up of the taxonomy module.
Comment #40.9
Lars Toomre CreditAttribution: Lars Toomre commentedDocumented creation of issue for clean up of system module.
Comment #40.10
Lars Toomre CreditAttribution: Lars Toomre commentedDocumented creation of issue for the clean up of the user module.
Comment #41
xenophyle CreditAttribution: xenophyle commentedMade an issue for the book module: #1327484: Clean up API docs for book and noted that I took it in the list above.
Comment #41.0
xenophyle CreditAttribution: xenophyle commentedassign book module to myself
Comment #41.1
jhodgdonadd missing issues to list and note ones that are sort of unclaimed
Comment #41.2
xjm(xjm) Added note about not killing kittens.
Comment #41.3
xjm(xjm) And, suggestion to use the issue summary.
Comment #41.4
Lars Toomre CreditAttribution: Lars Toomre commentedRecording that image and locale modules are no longer claimed by Lars Toomre.
Comment #42
sven.lauer CreditAttribution: sven.lauer commentedCreated issue for color module: #1332580: Clean up API docs for color module and added it to the list in the issue summary.
Comment #42.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed system module. See #1332580: Clean up API docs for color module.
Comment #43
sven.lauer CreditAttribution: sven.lauer commentedCreated #1332598: Clean up API docs for comment module and recorded this in the issue summary.
Comment #43.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed comment module: #1332598: Clean up API docs for comment module.
Comment #44
sven.lauer CreditAttribution: sven.lauer commentedCreated #1332636: Clean up API docs for contact module, and recorded this in the issue summary here.
Comment #44.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed contact module: #1332636: Clean up API docs for contact module
Comment #45
sven.lauer CreditAttribution: sven.lauer commentedCreated: #1332648: Clean up API docs for contextual module, and recorded this in the issue summary here.
Comment #45.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed contextual module: #1332648: Clean up API docs for contextual module
Comment #46
sven.lauer CreditAttribution: sven.lauer commentedCreated #1332658: Clean up API docs for dashboard module, and recorded this in the issue summary here.
Comment #46.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed dashboard module: #1332658: Clean up API docs for dashboard module
Comment #47
nicl CreditAttribution: nicl commentedJust an update to say that, as some rightfully predicted, the database directory is too large for me and I haven't had the time to make a proper go at it. So, for now, I'm unassigning myself from it. Sorry about this everyone.
Comment #47.0
nicl CreditAttribution: nicl commentedUnassigning myself (nicl) from the database directory as was too much.
Comment #47.1
jhodgdonmodify unassigning instructions
Comment #47.2
aspilicious CreditAttribution: aspilicious commentedAdded shortcut module issue
Comment #47.3
xjmUpdated issue summary.
Comment #48
xenophyle CreditAttribution: xenophyle commentedAssigned the issue for the field module: #1326634: Clean up API docs for field module to myself.
Comment #48.0
xenophyle CreditAttribution: xenophyle commentedassigning field module to myself
Comment #48.1
sven.lauer CreditAttribution: sven.lauer commentedClaimed Field_ui module
Comment #48.2
sven.lauer CreditAttribution: sven.lauer commentedClaimed File module #1347890: Clean up API docs for file module.
Comment #49
sven.lauer CreditAttribution: sven.lauer commentedClaimed (and submitted a patch for) File module: #1347890: Clean up API docs for file module
Claimed Filter module: #1347914: Clean up API docs for Filter module
Comment #49.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed filter.module #1347914: Clean up API docs for Filter module
Comment #50
sven.lauer CreditAttribution: sven.lauer commentedClaimed system.module .
Comment #50.0
sven.lauer CreditAttribution: sven.lauer commentedClaimed system.module
Comment #50.1
bartlantz CreditAttribution: bartlantz commentedMade an issue for the profiles directory: http://drupal.org/node/1355362 and noted that I took it in the summary.
Comment #51
Sree CreditAttribution: Sree commentedclaimed search module #1355670: Clean up API docs for search module
Comment #51.0
Sree CreditAttribution: Sree commentedclaimed search module #1355670
Comment #52
surendramohan CreditAttribution: surendramohan commentedclaimed path module #1355682: Clean up API docs for path module
Comment #53
surendramohan CreditAttribution: surendramohan commentedAccidently the "Title" I specified got wrong. How do I update the title section ?
Comment #54
webchickNo worries! You can just leave a comment that changes the "Issue title" field and it'll update the parent.
Comment #54.0
webchickClaiming path module - #1355682: Clean up API docs for path module
Comment #54.1
surendramohan CreditAttribution: surendramohan commentedClaimed Poll Module #1355712: Clean up API docs for Poll Module
Comment #55
surendramohan CreditAttribution: surendramohan commentedClaimed Poll Module #1355712: Clean up API docs for Poll Module
Comment #55.0
surendramohan CreditAttribution: surendramohan commentedClaimed Poll Module #1355712: Clean up API docs for Poll Module
Comment #56
xenophyle CreditAttribution: xenophyle commentedAssigned #1326672: Clean up API docs for menu module to myself.
Comment #57
jhodgdonJust want to say a big THANK YOU to all of you who have been contributing and reviewing patches! I think this is going really well and I'm very pleased. :)
Comment #57.0
jhodgdonassigned menu module to myself
Comment #58
xenophyle CreditAttribution: xenophyle commentedClaimed the forum module #1357138: Clean up API docs for forum module
Comment #58.0
xenophyle CreditAttribution: xenophyle commentedassign form module to myself
Comment #59
xenophyle CreditAttribution: xenophyle commentedClaimed the image module #1326608: Clean up API docs for image module
Comment #59.0
xenophyle CreditAttribution: xenophyle commentedassigning image module to myself
Comment #60
xenophyle CreditAttribution: xenophyle commentedClaimed the locale module #1326618: Clean up API docs for locale module.
Comment #61
xjmI just encountered an issue where differences in a test docblock between D7 and D8 prevented me from rerolling a patch for backport, and so I had to recreate the patch manually. It's not too late to backport some of the doc cleanups for the fixed issues, and I'm thinking this might actually be a good idea in many (though not all) cases. We will nearly always be rolling D7 patches against D8 first anyway.
I suggest we evaluate each patch on a case-by-case basis, and backport as much as is easy and not version-specific (and only things that are not new docs standards we've added for D8 like the datatypes).
Comment #62
jhodgdon+1 from me -- it was webchick who was against the idea. I think having properly-formatted docs is a plus for any version of Drupal, and I don't think any of the standards we are updating to here are version specific (we're specifically not doing the data types).
Anyway: webchick, what say you? Maybe take a look at some of the patches on fixed issues above and see what you think? Most of the cleanup has been "make it 80 characters", "make the verb tense right", and "this doc is totally wrong, let's get it right" stuff.
Comment #62.0
jhodgdonassign locale to myself
Comment #62.1
chris.leversuch CreditAttribution: chris.leversuch commentedClaim php module
Comment #63
webchickYeah, I'm fine (and have always been fine) with fixes that bring D7 inline with already-existing D7 standards. What I'm not fine with is introducing new rules, like the new way to document menu callbacks, including data types in @param, etc. I don't want the DX for D7 to go down for "normal" consumers/browsers of Drupal's API by introducing inconsistency in the PHPDoc as these major refactorings are gradually being rolled out throughout the code base. I have committed a few of the coding standards clean-up patches to D7 when they didn't mix the two.
The other thing I'd say is that now that we have Coder module integration with testbot, I'd love to see it made a priority to get these rules automated so it's clear to everyone what the standard is, and so that we don't end up having to re-do this enormous effort all over again before release.
Comment #64
jhodgdonSounds like a good plan. So we should:
a) Figure out specifically which types of changes we're doing in these patches should and should not be backported.
b) Reopen the issues above currently marked "fixed", and mark "backport" instead, with a comment from (a) in them explaining which parts are to be backported.
c) Figure out which d7/8 docs standards are not part of Coder, and which can be automatically tested, and file issues in Coder to get them added to the appropriate versions of Coder.
Just after adding this comment, I will edit the issue summary and add this to the "tasks" section.
Comment #64.0
jhodgdonClaim toolbar module
Comment #64.1
jhodgdonAdd tasks for backporting and coder to remaining tasks list
Comment #64.2
agentrickardTaking syslog patch.
Comment #65
jhodgdonTo all of you wonderful, generous participants in this sprint: We have a small backlog of 7 issues at the moment in this sprint that have patches that need to be reviewed:
http://drupal.org/project/issues/search/drupal?status[]=8&issue_tags=doc...
I've been really really busy the last week or so, and haven't been able to make time to review them... Anyone who's been participating here can take the next step and become a patch reviewer! Just check and see that the bullet points above in the issue summary have been taken care of, and that the changes in the patch are correct. Read through any added documentation and make sure it is clear and uses correct grammar/punctuation (my favorite nitpick is that "id" is a psychological term, while "ID" means identifier). Then either add some specific and constructive comments and mark the patch "needs work ", or if it's perfect, mark it "RTBC". If you are not sure, review what you can and leave a comment saying what you weren't sure about.
Thanks for your help! We're making great progress on bringing all the docs up to standards -- and I think if all or most of our existing doc is up to standards, it helps people write new docs that follows the standards (people will tend to copy/paste examples when writing new doc).
Comment #65.0
jhodgdonSpelling my own username correctly.
Comment #65.1
chris.leversuch CreditAttribution: chris.leversuch commentedClaim trigger.module
Comment #66
chris.leversuch CreditAttribution: chris.leversuch commentedWhat's the D7 standard for documenting a hook_menu() callback? http://drupal.org/node/1354#menu-callback just states the D8 standard.
Comment #66.0
chris.leversuch CreditAttribution: chris.leversuch commentedClaim includes/filetransfer directory
Comment #67
jhodgdonThere is no d7 standard. Sorry. We didn't have one, so adopted a new one for d8.
Comment #68
jhodgdonJust to clarify: the philosophy is that we are cleaning up d8 documentation to conform to d8 standards, but when we backport to d7, we only clean up areas that violate d7 standards, and leave the rest alone.
Comment #68.0
jhodgdonClaimed includes/database directory
Comment #69
xenophyle CreditAttribution: xenophyle commentedA new module, language.module, has been factored out of locale.module. I have been working on locale.module, so I will assign #1392962: Clean up API docs for the language module to myself and migrate my changes as appropriate.
Comment #70
synth3tk CreditAttribution: synth3tk commentedClaimed the overlay module: #1398404: Clean up API docs for overlay module
Comment #70.0
synth3tk CreditAttribution: synth3tk commentedAdding the new language module and assigning it to myself, since it seems to be composed of code from locale.module, which I have been working on.
Comment #70.1
synth3tk CreditAttribution: synth3tk commentedsynth3tk claimed the overlay module
Comment #71
geoffreyr CreditAttribution: geoffreyr commentedClaimed the openid module: #1405948: Clean up API docs for openid
Comment #72
synth3tk CreditAttribution: synth3tk commentedI can no longer work on overlay. Life decided to take over my time.
#1398404: Clean up API docs for overlay module
Comment #72.0
synth3tk CreditAttribution: synth3tk commentedAdded link to meta-issue for openid docs
Comment #73
Alan D. CreditAttribution: Alan D. commentedIs it worth a quick patch to fix some common issues in a single hit, like argument option lists with quotes? Reason: This can be done programmatically reducing the complication of the other patches that need to be reviewed.
For example, these to regexp found and (after a quick skim of the results) found nearly 400 corrections. Added the diff as a text file for example only. Time taken: 20min
" \*(\s*)\- '([^']*)'\:" | " \*(\s*)\- \"([^\"]*)\"\:" with the replacement " \*$1\- $2\:" via Eclipse Replace
Skipped a couple of arguments in entity.query.inc ('IN', etc) and common.inc ('!IE', etc)
Comment #74
jhodgdonThere is a separate meta-issue for list-related formatting issues:
#1331240: [Meta] Correct documentation for 'list' related issues
I think this patch and/or script would be welcome there. :)
Comment #74.0
jhodgdonUpdated issue summary.
Comment #74.1
Cameron Tod CreditAttribution: Cameron Tod commentedUpdated issue summary.
Comment #75
jhodgdonI just updated two issues that have been unassigned in the summary.
Comment #76
pillarsdotnet CreditAttribution: pillarsdotnet commentedWhere would be it appropriate to post a one-line command to remove all trailing spaces?
Comment #78
jhodgdonYou can edit the issue summary and put it there (in the step-by-step procedures). I don't know of any other good place to post it, except it should probably be part of some Coder module? Thanks!
Comment #79
Cameron Tod CreditAttribution: Cameron Tod commentedI have added a patch for user.module here: http://drupal.org/node/1326666#comment-5520832
Let me know if its ok and I can move onto other modules.
Comment #79.0
Cameron Tod CreditAttribution: Cameron Tod commentedtwo issues unassigned
Comment #80
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch to remove all trailing spaces, generated with
EDIT: Note that patch passes test (as it should; being a whitespace-only change) but needs to be regenerated as necessary.
Comment #81
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #81.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedOne-line script to remove trailing spaces.
Comment #82
kotnik CreditAttribution: kotnik commentedCreated issue #1418980: Clean up API docs for rdf and added it to the list in the issue summary.
Comment #83
jhodgdon@pillarsdotnet -- +1 on the patch on #8. I didn't realize you were going to attach a patch.... Maybe we can put it on a separate issue so I can feel more comfortable marking it RTBC? I'd like to leave this meta issue as "active" until it's done (as you realized in #81).
Comment #83.0
jhodgdonTook rdf module.
Comment #84
pillarsdotnet CreditAttribution: pillarsdotnet commented@Jhodgdon -- See #1419298: Remove all trailing whitespace from Drupal core files.
Comment #84.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded issue for trailing whitespace.
Comment #85
jhodgdonI just filed issues for all the missing ones in the above list. We are making a lot of progress on this, yeah!!! I am also going to go through the existing unassigned issues and make sure they are all tagged "Novice" so people will find them, and also ping people who have assigned issues but not done anything on them recently to see if we should re-assign.
Comment #85.0
jhodgdonFiled issues for the missing ones
Comment #85.1
jhodgdondblog is now unassigned
Comment #86
sven.lauer CreditAttribution: sven.lauer commentedUnassigned Filter module. #1347914: Clean up API docs for Filter module has a partial patch, so if anyone wants quick(ish) gratification ...
Comment #87
xjmFYI to everyone, we are currently well over code slush thresholds for core, which means that large cleanups like our docs cleanups shouldn't be committed in order to not interfere with fixing major and critical issues. So, you might want to hold off on rolling your patches until we can get the issues back down under the thresholds; I personally will be doing so. (You can see the count at any time by looking at the contributor block on the dashboard; there should be less than 15 criticals and less than 100 majors in both categories.) Thanks!
Comment #88
achtonxjm: So does that mean that we can still take on issues and clean up the code, but need to watch relevant code changes ourselves until there is room in the critical/major queues and then roll patches?
Seems like the thresholds are having a negative impact on this initiative, then :-/
Comment #89
jhodgdonachton: NO! :) We are on track. The thresholds were a temporary problem -- bugs come and go, and we are over/under thresholds regularly. Aside from a few key spots, like the System module and the include files, it's not having a huge impact on the ability to commit these patches. If you choose a less-central module or theme, you should be OK.
Comment #90
achtonExcellent, thanks for clearing that up :)
Comment #91
Cameron Tod CreditAttribution: Cameron Tod commentedOK, so I take that as meaning we should carry on with our patches? I will do some more work on the user module stuff this weekend if so.
Comment #92
jhodgdonYes, carry on! Sorry, I didn't realize that xjm's comment above was slowing progress, or I would have said something earlier. :)
Comment #92.0
jhodgdonUpdated issue summary.
Comment #93
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI made an issue for replacing tabs with spaces. This mostly affects the misc folder but not exclusively. Also I did not do the modules folder.
The new issue is #1513540: Replace tabs with spaces from Drupal core files..
Comment #94
jhodgdon#93 - note: that issue was not really part of the API docs cleanup, and it was closed as "won't fix" because it affected only files that are 3rd-party libraries, which we don't touch.
Comment #94.0
jhodgdonUpdated issue summary.
Comment #94.1
jhodgdonremove trailing whitespace issue, not really part of this effort
Comment #94.2
TravisCarden CreditAttribution: TravisCarden commentedReformatted task list as table. (Hopefully no one will mind; I find it a lot more skimmable.)
Comment #94.3
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #94.4
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #94.5
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #94.6
xjmUpdated issue summary.
Comment #94.7
xjmUpdating for #1299424: Allow one module per directory and move system tests to core/modules/system.
Comment #94.8
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #94.9
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #94.10
TravisCarden CreditAttribution: TravisCarden commentedAdded new "files in core directory" issue and organized table a little.
Comment #94.11
jhodgdonupdate assignment status of issues
Comment #94.12
jhodgdonmore status updates
Comment #94.13
markgifford CreditAttribution: markgifford commentedAdded my username to the issue #1310084 in the summary table
Comment #94.14
jhodgdonAdd xmlrpc module issue to list (newly-added module)
Comment #94.15
markgifford CreditAttribution: markgifford commentedRemoving my name from the table following unassigning of issue #1606946
Comment #95
CBI have claimed #1431670: Clean up API docs for system/tests, E-I
Comment #96
CBHey all, just uploaded a small patch (in #1431670) to gather feedback about whether I am going about it all the right way.
If you have time, I'd love some feedback.
Thanks
Comment #96.0
CBJust adding my name next to my assigned issue.
Comment #96.1
mallezieadded myself for image cleanup
Comment #97
mmartinov CreditAttribution: mmartinov commentedI have assigned #1431664: Clean up API docs for system/tests, J-Z, including subdirectories to myself.
Comment #98
xjmSo, after close to a year of working with (and then not working with) these large cleanup patches, my perspective is that we will be more successful if we narrow the scope per patch. Mixing multiple different kind of documentation cleanup into a single patch makes them difficult to manage and extremely time-consuming to review, especially when we also polish the documentation language alongside pure code style fixes. I think it might be better to make incremental changes and commit what fixes we can first, and then iterate on them for further cleanups.
Comment #99
webchickI agree with that assessment.
I also wonder though if it would be better to postpone this initiative until after Dec. 1, when there's a 5-month cycle explicitly for things like this? I know at least Dries, catch, and I would love to be able to focus on committing big feature patches between now and then, and huge clean-up patches can sometimes cause conflicts making them scary to commit when they might invalidate something "needle moving." We do have Jennifer as a docs maintainer who can help streamline things, so that's good, but it does nevertheless split focus and add more things to review in the RTBC queue.
Anyway, just a thought.
Comment #100
jhodgdonI'd be happy to review/commit patches that, say, just did "Add the blank lines between param/return" and then wait for another fix for some other aspect of the issue. I think we should continue to stick to the "one issue per directory" thing though, rather than, say, "add blank lines between param/return everywhere in core", as those are *really* disruptive.
Actually, lately I have been slowly getting through the CNR patches in this initiative and committing any that didn't have major problems, then setting them to follow-up to fix anything left over that I saw -- which is a departure from the earlier review nitpicks... there are still a few in the queue that I need to review; some of them may even still apply. It takes a while to get through them of course, and I've been busy writing this book and with summer vacations, etc. So the patches are languishing.
I'm also of course being careful not to commit things that conflict with the "avoid commit conflict" issues, so hopefully the patches I've committed haven't caused any really major headaches?
Comment #101
webchickYeah, fair enough. What I'll start doing is just assigning RTBC issues to you (already started doing this last night) so I quit looking at the RTBC count and getting hives. :D Sound good?
Comment #102
Lars Toomre CreditAttribution: Lars Toomre commented@jhodgdon Just to be clear, let's say that I am going to work further on the big file common.inc. Further, let's say I working on list indentation.
Do you want me to re-open #1315886: Clean up API docs for includes directory, files starting with A-C with those incremental changes or should I start a new issue? Do I need to include fixed for all of the includes A-C or are just the fixes in all of common.inc approriate with the thoughts in #98?
Comment #103
jhodgdonWhat we don't want to have is a patch for each file in core in any initiative -- that would be thousands of patches, which is why we split up this initiative the way we did (hopefully to make each patch somewhat manageable, which may or may not have succeeded). common.inc is huge, though... I don't know what to tell you except "use your own judgement".
And regarding the list doc formatting cleanup, yes I think I would open up those issues again and add a new patch.
Comment #103.0
jhodgdonI have assigned #1431664: Clean up API docs for system/tests, J-Z, including subdirectories to myself.
Comment #103.1
xjmUpdated issue summary.
Comment #103.2
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.3
Lars Toomre CreditAttribution: Lars Toomre commentedAdded sub-issue for Test base classes in simpletest module.
Comment #103.4
Lars Toomre CreditAttribution: Lars Toomre commentedAdded aggregator continuation sub-issue.
Comment #103.5
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.6
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.7
Lars Toomre CreditAttribution: Lars Toomre commentedAdded continuation sub-issue for book module.
Comment #103.8
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.9
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.10
Lars Toomre CreditAttribution: Lars Toomre commentedRemoving myself from Simpletest sub-issue.
Comment #103.11
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.12
Lars Toomre CreditAttribution: Lars Toomre commentedCorrection of formatting for additional Color follow-up issue.
Comment #103.13
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.14
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.15
Lars Toomre CreditAttribution: Lars Toomre commentedAdded continuation sub-issue for Contact module.
Comment #103.16
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.17
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.18
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.19
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #103.20
Lars Toomre CreditAttribution: Lars Toomre commentedRemoved duplicate addition for Statistics follow up issue.
Comment #104
jhodgdonJust a note... When updating the issue summary on this issue, please put in a more descriptive log message than "Updated the issue summary". The revision log right now is nearly useless.
Comment #105
webchickThat's Dreditor doing that. :\ I hate that behaviour too. :\
Comment #106
jhodgdonWhat? Dreditor lets you edit the issue summary and fills in a meaningless log message? UGGGGGGG!!!! Should we file a Dreditor issue? (I personally don't use Dreditor, but won't get into that here.)
Comment #106.0
jhodgdonUpdated issue summary.
Comment #107
webchickIt adds a "fast edit" link to the issue summary with no log message field, and that's what it fills in. And yes, go ahead and file a dreditor issue.
Comment #107.0
webchickCleaned up sub-issues assigned to Lars Toomre.
Comment #107.1
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #108
CBI've removed myself from assignment on E-I as I am working on the accessibility of D8 also and really dont have time for both.
Comment #109
YesCT CreditAttribution: YesCT commentedcoming here from the examples being added for the coding standards,
in http://drupal.org/node/1918356/revisions/view/2571204/2571928
- #1326634-66: Clean up API docs for field module follow-up to change the verb tenses will help make some examples match the code
like: grep -R "Install, update and uninstall functions for the system module." *
core/modules/system/system.install: * Install, update and uninstall functions for the system module.
- changing System to uppercase might be covered in #1326664: Clean up API docs for system module (excluding subdirectories) in the part:
Make incidental style and grammar corrections only to those docblocks already being updated.
- for verb, adding @see and @ingroup, I dont see an issue that might fall under.
$ grep -R "Default theme implementation to display a block." *
core/modules/block/templates/block.tpl.php: * Default theme implementation to display a block.
I might find a few more, and figure out which of these follow-ups already covers them.
Comment #109.0
YesCT CreditAttribution: YesCT commentedRemoving myself from assignment to E-I
Comment #110
jhodgdonWell... This issue got a lot of work done, and it took a lot of energy, which neither I nor anyone else seems to have for it any more. I'm just going to close the rest of this meta-issue as "won't fix" at this point, and the individual issues that are still open at this point. I'd prefer to see energy directed towards more fruitful tasks, like fixing factual errors and really confusing docs.
Thanks to all who participated, and I hope you are not afraid of working on Documentation patches forever!