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:

  1. Choose an unclaimed module or directory from the Remaining Tasks section below.
  2. 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
  3. 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).
  4. 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.
  5. 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.

  6. 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".
  7. Start over at the first step if desired!
  8. 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.)

Module/Directory Assigned Issue link
core
files in core directory #1606946: Clean up API docs for files in core directory
core/includes
includes/database directory #1316902: Clean up API docs for lib/Drupal/core/Database
includes/filetransfer directory #1382222: Clean up API docs for includes/filetransfer
includes directory, files starting with A-C (~14k lines) xjm #1315886: Clean up API docs for includes directory, files starting with A-C
includes directory, files starting with D-G* xjm #1317620: Clean up API docs for includes directory, files starting with D-G
includes directory, files starting with H-M (~10.5k lines) #1317626: Clean up API docs for includes directory, files starting with H-M
includes directory, files starting with N-Z (~10k lines) #1317628: Clean up API docs for includes directory, files starting with N-Z
core/modules
action module #1787900: Clean up cruft in the Action documentation
aggregator module xenophyle #1325116: Clean up API docs for Aggregator module
#1807556: Further clean up of API docs for Aggregator module
ban module #1807612: Clean up API docs for Ban module
block module xenophyle #1323720: Clean up API docs for block module
#1807652: Further clean up of API docs for Block module
book module xenophyle #1327484: Clean up API docs for book
#1807688: Further clean up of API docs for Book module
breakpoint module
color module sven.lauer #1332580: Clean up API docs for color module
#1808178: Further clean up of API docs for Color module
comment module #1332598: Clean up API docs for comment module
config module #1541930: Clean up API docs for config module
contact module sven.lauer #1332636: Clean up API docs for contact module
#1811628: Further clean up of API docs for Contact module
contextual module sven.lauer #1332648: Clean up API docs for contextual module
dashboard module sven.lauer #1332658: Clean up API docs for dashboard module
dblog module #1326600: Clean up API docs for dblog module
entity module #1315340: Clean up API docs for entity.inc
field module #1326634: Clean up API docs for field module
field_ui module sven.lauer #1326638: Clean up API docs for field_ui module
file module sven.lauer #1347890: Clean up API docs for file module
#1811674: Further clean up of API docs for File module
filter module #1347914: Clean up API docs for Filter module
forum module jstoller #1357138: Clean up API docs for forum module
help module rc_100 #1319904: Clean up API docs for help module
image module mallezie #1326608: Clean up API docs for image module
language module #1392962: Clean up API docs for the language module
locale module #1326618: Clean up API docs for locale module
menu module #1326672: Clean up API docs for menu module
node module Albert Volkman #1313980: Clean up API docs for node module
openid module #1405948: Clean up API docs for openid
overlay module #1398404: Clean up API docs for overlay module
path module surendramohan #1355682: Clean up API docs for path module
#1811948: Further clean up of API docs for Path module
php module chris.leversuch #1367000: Clean up API docs for php module
poll module batigolix #1355712: Clean up API docs for Poll Module
rdf module #1418980: Clean up API docs for rdf
search module #1355670: Clean up API docs for search module
shortcut module #1339216: Clean up API docs for shortcut module
simpletest module** #1431636: Clean up API docs for simpletests module (excluding files subdirectory)
simpletest base test classes #1805264: Clean up API docs for Test base classes in Simpletest module
statistics module NROTC_Webmaster #1313510: Clean up API docs for statistics module
#1812008: Further clean up of API docs for Statistics module
syslog module agentrickard #1368872: Clean up API docs for syslog
system module (excluding subdirectories) #1326664: Clean up API docs for system module (excluding subdirectories)
system module, tests subdirectory A-D #1431658: Clean up API docs for system module tests, A-D, including subdirectories
system module, tests subdirectory E-I #1431670: Clean up API docs for system/tests, E-I
system module, tests subdirectory J-Z mmartinov #1431664: Clean up API docs for system/tests, J-Z, including subdirectories
taxonomy module xjm #1326644: Clean up API docs for taxonomy module
toolbar module chris.leversuch #1367014: Clean up API docs for toolbar module
tracker module NROTC_Webmaster #1315214: Clean up API docs for the tracker module
#1813272: Further clean up of API docs for Tracker module
translation module NROTC_Webmaster #1431632: Clean up API docs for translation module
trigger module chris.leversuch #1379126: Clean up API docs for trigger.module
update module Tor Arne Thune #1431634: Clean up API docs for update module
user module #1326666: Clean up API docs for user module
xmlrpc module #1649140: Clean up API docs for xmlrpc module
core/themes
themes directory #1431630: Clean up API docs for themes directory
profiles
profiles directory bartlantz #1355362: Clean up API docs for profiles directory

* 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:

  1. 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).
  2. 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.
  3. 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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue tags: +docs-cleanup-2011

I 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.

jhodgdon’s picture

Issue tags: +Novice

Oh 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.

jhodgdon’s picture

Issue summary: View changes

Complete the description of what to do

sven.lauer’s picture

Nice. 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?

jn2’s picture

I plan to take on some of this as well. Might start with the node module, when I get a chance.

jhodgdon’s picture

RE $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.

sven.lauer’s picture

Okay. 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?).

jhodgdon’s picture

If 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.

jhodgdon’s picture

Issue summary: View changes

Add items to not do in this sprint in large doses

aenw’s picture

Issue summary: View changes

marked the statistics module as taken

jn2’s picture

Made the issue for node module: #1313980: Clean up API docs for node module and added to issue summary.

aenw’s picture

Made an issue for the statistics module: #1313510: Clean up API docs for statistics module and noted that I took it in the list above.

aenw’s picture

Made 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.

aenw’s picture

Issue summary: View changes

Added jn2, node module

aenw’s picture

Made an issue for the entity module: #1315340: Clean up API docs for entity.inc and noted that I took it in the list above.

aenw’s picture

Issue summary: View changes

adding formatting to the issue links for the node and statistics module, and noted that I'm taking the tracker module.

aenw’s picture

Issue summary: View changes

marked the entity module as taken, issue #1315340

jhodgdon’s picture

I 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. :)

jhodgdon’s picture

Issue summary: View changes

Removed code tags so we could see issue links. :)

xjm’s picture

jhodgdon’s picture

Wow, 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.

xjm’s picture

As part of this sprint can/should we correct @param declarations if they are missing (optional) and Defaults to foo. for parameters with provided defaults?

What about general violation of the 80-character limit in docblocks?

jhodgdon’s picture

optiona/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.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

optiona/defaults: You can. It does make the patch a slight bit harder to review quickly.

Alright; since this is a pretty fat cleanup to begin with (common.inc baby!), I'll file such corrections as separate issues.

jhodgdon’s picture

Just filed:
#1315992: No standard for documenting menu callbacks

We may need to get that resolved before cleanup patches are done!

jhodgdon’s picture

Issue summary: View changes

Add note about line wrapping as an item to fix.

nicl’s picture

I 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

jhodgdon’s picture

You 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.

jhodgdon’s picture

Issue summary: View changes

Have created an issue for includes/database so am adding this information to the summary along with issue link.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Since 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.

xjm’s picture

Aw 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. :)

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

I wonder if your emacs macros could be turned into some scripts that others could use to automate some of the process?

xjm’s picture

Hmm, well, they're not that smart and require user interaction. However, the following parts of the cleanup are hypothetically scriptable:

  • Ensure that all lines in a file beginning with whitespace followed by a * are under 80 characters.
  • Search for @return and confirm that wherever it is found, the line above contains only whitespace followed by a *.
  • Search for @see and @ingroup and confirm that, until the next line that is whitespace followed by */, 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.

xjm’s picture

FileSize
2.03 KB
  1. Stick the attached in your Drupal root and rename it back to parse.php.
  2. Edit the file and enter your module name on the line indicated.
  3. Run 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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Oh, 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?

jhodgdon’s picture

The 80-character limit can be ignored in @code areas.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Issue summary: View changes

Update to include standard on menu callbacks, and also not backporting to Drupal 7

rc_100’s picture

Issue summary: View changes

Made the issue for help module: #1319904: Clean up API docs for help module and added to the issue summary.

jhodgdon’s picture

Issue summary: View changes

Remove idea of unassigning when you attach a patch

xjm’s picture

Regarding #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.

jhodgdon’s picture

Do the 81-character lines (by your count) wrap in your editor? They don't in mine (emacs). Would like some other opinions...

xjm’s picture

#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.

jhodgdon’s picture

That 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?

xjm’s picture

Tried coder, but it apparently doesn't check character count.

webchick’s picture

Can 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.

jhodgdon’s picture

The 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.

jhodgdon’s picture

I 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)

jhodgdon’s picture

This one too:
#521452: Update to check for new function header doc standards

So I think the issues have been filed.

xjm’s picture

FileSize
2.12 KB

Don'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). ;)

xjm’s picture

Issue summary: View changes

Add to do item to format param/return statements

xenophyle’s picture

Made an issue for the block module: #1323720: Clean up API docs for block module and noted that I took it in the list above.

xenophyle’s picture

Issue summary: View changes

assigned block module to myself

xenophyle’s picture

Made an issue for the aggregator module: #1325116: Clean up API docs for Aggregator module and noted that I took it in the list above.

xjm’s picture

Edit: Never mind; I was mistaken. Click on the "core" tab to review core includes in coder.

xjm’s picture

Issue summary: View changes

assigning aggregator module to myself

jn2’s picture

Issue summary: View changes

Note about issue status

jn2’s picture

Issue summary: View changes

Status note no longer needed.

Lars Toomre’s picture

Issue summary: View changes

Assigning clean up of dblog module to Lars Toomre

Lars Toomre’s picture

Issue summary: View changes

Assign the clean up of image module to Lars Toomre.

Lars Toomre’s picture

Issue summary: View changes

Assign clean up of locale module to Lars Toomre.

Lars Toomre’s picture

Issue summary: View changes

Added issue for clean up of the field module.

Lars Toomre’s picture

Issue summary: View changes

Added issue reference for clean up of field_ui module.

Lars Toomre’s picture

Issue summary: View changes

Documented creation of issue for clean up of the taxonomy module.

Lars Toomre’s picture

Issue summary: View changes

Documented creation of issue for clean up of system module.

Lars Toomre’s picture

Issue summary: View changes

Documented creation of issue for the clean up of the user module.

xenophyle’s picture

Made an issue for the book module: #1327484: Clean up API docs for book and noted that I took it in the list above.

xenophyle’s picture

Issue summary: View changes

assign book module to myself

jhodgdon’s picture

Issue summary: View changes

add missing issues to list and note ones that are sort of unclaimed

xjm’s picture

Issue summary: View changes

(xjm) Added note about not killing kittens.

xjm’s picture

Issue summary: View changes

(xjm) And, suggestion to use the issue summary.

Lars Toomre’s picture

Issue summary: View changes

Recording that image and locale modules are no longer claimed by Lars Toomre.

sven.lauer’s picture

Created issue for color module: #1332580: Clean up API docs for color module and added it to the list in the issue summary.

sven.lauer’s picture

Issue summary: View changes
sven.lauer’s picture

Created #1332598: Clean up API docs for comment module and recorded this in the issue summary.

sven.lauer’s picture

Issue summary: View changes
sven.lauer’s picture

Created #1332636: Clean up API docs for contact module, and recorded this in the issue summary here.

sven.lauer’s picture

Issue summary: View changes
sven.lauer’s picture

Created: #1332648: Clean up API docs for contextual module, and recorded this in the issue summary here.

sven.lauer’s picture

Issue summary: View changes
sven.lauer’s picture

Created #1332658: Clean up API docs for dashboard module, and recorded this in the issue summary here.

sven.lauer’s picture

Issue summary: View changes
nicl’s picture

Just 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.

nicl’s picture

Issue summary: View changes

Unassigning myself (nicl) from the database directory as was too much.

jhodgdon’s picture

Issue summary: View changes

modify unassigning instructions

aspilicious’s picture

Issue summary: View changes

Added shortcut module issue

xjm’s picture

Issue summary: View changes

Updated issue summary.

xenophyle’s picture

Assigned the issue for the field module: #1326634: Clean up API docs for field module to myself.

xenophyle’s picture

Issue summary: View changes

assigning field module to myself

sven.lauer’s picture

Issue summary: View changes

Claimed Field_ui module

sven.lauer’s picture

Issue summary: View changes
sven.lauer’s picture

Claimed (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

sven.lauer’s picture

Issue summary: View changes
sven.lauer’s picture

Claimed system.module .

sven.lauer’s picture

Issue summary: View changes

Claimed system.module

bartlantz’s picture

Issue summary: View changes

Made an issue for the profiles directory: http://drupal.org/node/1355362 and noted that I took it in the summary.

Sree’s picture

Sree’s picture

Issue summary: View changes

claimed search module #1355670

surendramohan’s picture

surendramohan’s picture

Accidently the "Title" I specified got wrong. How do I update the title section ?

webchick’s picture

No worries! You can just leave a comment that changes the "Issue title" field and it'll update the parent.

webchick’s picture

Issue summary: View changes
surendramohan’s picture

Issue summary: View changes
surendramohan’s picture

surendramohan’s picture

Issue summary: View changes
xenophyle’s picture

jhodgdon’s picture

Just 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. :)

jhodgdon’s picture

Issue summary: View changes

assigned menu module to myself

xenophyle’s picture

xenophyle’s picture

Issue summary: View changes

assign form module to myself

xenophyle’s picture

xenophyle’s picture

Issue summary: View changes

assigning image module to myself

xenophyle’s picture

xjm’s picture

I 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).

jhodgdon’s picture

+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.

jhodgdon’s picture

Issue summary: View changes

assign locale to myself

chris.leversuch’s picture

Issue summary: View changes

Claim php module

webchick’s picture

Yeah, 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.

jhodgdon’s picture

Sounds 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.

jhodgdon’s picture

Issue summary: View changes

Claim toolbar module

jhodgdon’s picture

Issue summary: View changes

Add tasks for backporting and coder to remaining tasks list

agentrickard’s picture

Issue summary: View changes

Taking syslog patch.

jhodgdon’s picture

To 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).

jhodgdon’s picture

Issue summary: View changes

Spelling my own username correctly.

chris.leversuch’s picture

Issue summary: View changes

Claim trigger.module

chris.leversuch’s picture

What's the D7 standard for documenting a hook_menu() callback? http://drupal.org/node/1354#menu-callback just states the D8 standard.

chris.leversuch’s picture

Issue summary: View changes

Claim includes/filetransfer directory

jhodgdon’s picture

There is no d7 standard. Sorry. We didn't have one, so adopted a new one for d8.

jhodgdon’s picture

Just 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.

jhodgdon’s picture

Issue summary: View changes

Claimed includes/database directory

xenophyle’s picture

A 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.

synth3tk’s picture

synth3tk’s picture

Issue summary: View changes

Adding 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.

synth3tk’s picture

Issue summary: View changes

synth3tk claimed the overlay module

geoffreyr’s picture

Claimed the openid module: #1405948: Clean up API docs for openid

synth3tk’s picture

I can no longer work on overlay. Life decided to take over my time.
#1398404: Clean up API docs for overlay module

synth3tk’s picture

Issue summary: View changes

Added link to meta-issue for openid docs

Alan D.’s picture

Is 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)

jhodgdon’s picture

There 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. :)

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

Cameron Tod’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

I just updated two issues that have been unassigned in the summary.

pillarsdotnet’s picture

Where would be it appropriate to post a one-line command to remove all trailing spaces?

jhodgdon’s picture

You 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!

Cameron Tod’s picture

I 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.

Cameron Tod’s picture

Issue summary: View changes

two issues unassigned

pillarsdotnet’s picture

Status: Active » Needs review
FileSize
12.87 KB

Patch to remove all trailing spaces, generated with

perl -pi -e 's, *$,,' $(find core -regex '.*\.\(engine\|html\|inc\|info\|install\|js\|json\|module\|php\|script\|sh\|sql\|test\|txt\|xml\)')

EDIT: Note that patch passes test (as it should; being a whitespace-only change) but needs to be regenerated as necessary.

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary: View changes

One-line script to remove trailing spaces.

kotnik’s picture

Created issue #1418980: Clean up API docs for rdf and added it to the list in the issue summary.

jhodgdon’s picture

Status: Needs review » Active

@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).

jhodgdon’s picture

Issue summary: View changes

Took rdf module.

pillarsdotnet’s picture

pillarsdotnet’s picture

Issue summary: View changes

Added issue for trailing whitespace.

jhodgdon’s picture

I 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.

jhodgdon’s picture

Issue summary: View changes

Filed issues for the missing ones

jhodgdon’s picture

Issue summary: View changes

dblog is now unassigned

sven.lauer’s picture

Unassigned Filter module. #1347914: Clean up API docs for Filter module has a partial patch, so if anyone wants quick(ish) gratification ...

xjm’s picture

FYI 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!

achton’s picture

xjm: 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 :-/

jhodgdon’s picture

achton: 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.

achton’s picture

Excellent, thanks for clearing that up :)

Cameron Tod’s picture

OK, 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.

jhodgdon’s picture

Yes, carry on! Sorry, I didn't realize that xjm's comment above was slowing progress, or I would have said something earlier. :)

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

I 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..

jhodgdon’s picture

#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.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Issue summary: View changes

remove trailing whitespace issue, not really part of this effort

TravisCarden’s picture

Issue summary: View changes

Reformatted task list as table. (Hopefully no one will mind; I find it a lot more skimmable.)

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

TravisCarden’s picture

Issue summary: View changes

Added new "files in core directory" issue and organized table a little.

jhodgdon’s picture

Issue summary: View changes

update assignment status of issues

jhodgdon’s picture

Issue summary: View changes

more status updates

markgifford’s picture

Issue summary: View changes

Added my username to the issue #1310084 in the summary table

jhodgdon’s picture

Issue summary: View changes

Add xmlrpc module issue to list (newly-added module)

markgifford’s picture

Issue summary: View changes

Removing my name from the table following unassigning of issue #1606946

CB’s picture

CB’s picture

Hey 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

CB’s picture

Issue summary: View changes

Just adding my name next to my assigned issue.

mallezie’s picture

Issue summary: View changes

added myself for image cleanup

mmartinov’s picture

xjm’s picture

So, 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.

webchick’s picture

I 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.

jhodgdon’s picture

I'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?

webchick’s picture

Yeah, 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?

Lars Toomre’s picture

@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?

jhodgdon’s picture

What 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.

jhodgdon’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Added sub-issue for Test base classes in simpletest module.

Lars Toomre’s picture

Issue summary: View changes

Added aggregator continuation sub-issue.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Added continuation sub-issue for book module.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Removing myself from Simpletest sub-issue.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Correction of formatting for additional Color follow-up issue.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Added continuation sub-issue for Contact module.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Removed duplicate addition for Statistics follow up issue.

jhodgdon’s picture

Just 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.

webchick’s picture

That's Dreditor doing that. :\ I hate that behaviour too. :\

jhodgdon’s picture

What? 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.)

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

It 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.

webchick’s picture

Issue summary: View changes

Cleaned up sub-issues assigned to Lars Toomre.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

CB’s picture

I'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.

YesCT’s picture

coming 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.

YesCT’s picture

Issue summary: View changes

Removing myself from assignment to E-I

jhodgdon’s picture

Issue summary: View changes
Status: Active » Closed (won't fix)

Well... 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!