http://code.google.com/p/google-highly-open-participation-drupal/issues/...
Categories: Code, QualityAssurance
Proposed by: Crell
This task is to select three (3) modules in Drupal core and ensure that all functions, classes, methods, and constants in it have properly formed, complete Doxygen code comments.
There are many forms of documentation. API documentation, that is, documentation on how certain pieces of code should be called by other pieces of code, is important to the future of any system but especially one with as diverse a development team as Drupal. Both new and experienced developers frequently need to refer back to API documentation in order to develop new features or fix bugs, and having clear, correct documentation is critical for that.
To complete this task, review three (3) modules in Drupal core and add API documentation to any function, class, method, or constant that does not have it or expand on existing documentation where it is incomplete, unclear, or does not follow established Drupal standards. Drupal currently uses the Doxygen comment standard.
The final deliverable will be a patch against the 6.x version of Drupal posted to the issue queue that has been reviewed and marked "Ready to be Committed". See the resources section for more information.
Resources:
* "Coding resources" section of Wiki
* http://www.stack.nl/~dimitri/doxygen/docblocks.html
* http://www.stack.nl/~dimitri/doxygen/commands.html
Estimated time:
2 days
Comment | File | Size | Author |
---|---|---|---|
#27 | api-doc-reroll-3.patch | 52.96 KB | vitezslav.smid |
#22 | api-doc-reroll-2.patch | 53.33 KB | vitezslav.smid |
#21 | api-doc-reroll.patch | 53.37 KB | JirkaRybka |
#20 | drupal-6.x-dev-api-documentation-v4-edit.patch | 53.74 KB | vitezslav.smid |
#18 | drupal-6.x-dev-api-documentation-v3-edit.patch | 56.03 KB | vitezslav.smid |
Comments
Comment #1
webchickTask claimed by onetrap (couldn't find d.o user name)
Comment #2
Crell CreditAttribution: Crell commentedSubscribing.
Comment #3
webchickI'm behind on my task updates. :P Unfortunately the deadline ran out om the old claimee, and this was later claimed by vitezslav.smid, who has posted a patch to http://code.google.com/p/google-highly-open-participation-drupal/issues/... which needs review. I'll ask to cross-post here as well.
Comment #4
vitezslav.smid CreditAttribution: vitezslav.smid commentedIt was much more work than I had anticipated, but here it is at last. It's a patch
against "drupal-6.0-beta4" root directory. I hope it will be of any use :)
Comment #5
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedThis is so nice! :-) ... and it's ready!
Comment #6
webchickLet's get a couple sets of eyes on this first.
Comment #7
Crell CreditAttribution: Crell commentedA quick read through of the text makes most of it look good. However, I did notice a few problems.
- The comment blocks do not conform to Drupal standard. They should read like this:
Note that description text is on the following line, indented 2 spaces, rather than being on the same lin.e
- Hook implementations should be marked "Implementation of hook_menu()", which it looks like you did, but currently do not use @see hook_menu(). There's currently no actual function called hook_menu(), so we shouldn't @see it. At some point in the future we may move toward doing that, but currently it's not the standard. (Ibid for all other hooks, of course.)
- The function parameters to hook implementations are also not needed, as those are standardized. Just the "Implementation of hook_foo()" string is sufficient. Similarly, form definition functions should be @ingroup forms, but do not need their parameters or return defined (unless there's a non-standard parameter being passed in, like to the form builder function itself). Those are standard for all forms.
- Don't over-use @see. There's no hard-and-fast rule about using @see yet, but in general it should only be used if the function being referenced would only ever be used as part of the function referencing it. For example, form validate and submit handlers have no actual use except as part of a form, so the form builder function @see's them so that going to the form builder's API page will link to the validate and submit handlers. Where else it should be used is not well defined, but I'd say err on the conservative side. No sense adding tags that we're just going to take out later down the line when we do establish a standard.
- Functions should always be separated from the next/previous function (and/or its DocBlock) by a single carriage return. I think a few multi-line breaks crept in there.
- The first line of each DocBlock should be a short and sweet single line, no more than a sentence. Additional descriptive text, which is good where appropriate, should be separated from the first line by one blank line in the DocBlock. That way there's a short description that is good for lists, and a longer description (which could be one line or several paragraphs, as needed) that can be pulled up for more information.
Comment #8
Gábor HojtsyThe @return docs should also be on a separate line, just as with @param.
Comment #9
vitezslav.smid CreditAttribution: vitezslav.smid commentedI apologize for the mistakes. Here is a revised version, I really hope it's going to be OK this time.
Comment #10
webchickMarking back for review.
Comment #11
drewish CreditAttribution: drewish commentedI tried applying the patch but it failed on the comment.module, system.admin.inc and system.module.
You're getting very close but I think some of Crell's comments in #7 still apply:
- It's minor but the formatting of the @params is still incorrect. The variable name should be on the first line with @param, and the description by itself on the second. Perhaps it would be helpful to look at the Doxygen formatting conventions.
- There are quite a few @see's that don't seem very helpful, e.g. the
@see _aggregator_page_list()
on aggregator_page_source()'s comment is probably actually a bad idea. Documenting the implementation leads people to believe that's the only way it can be done and ties you to a single implementation.- aggregator_init() is an implementation of hook_init() and should be documented as such.
And, you added a few @ingroup forms, but it looks like you missed aggregator's forms.
Comment #12
drewish CreditAttribution: drewish commentedComment #13
Crell CreditAttribution: Crell commentedNo need to apologize, vitezslav. You're not really a Drupal developer until you've totally and inexcusably screwed up on something at least three times. :-) At first glance, this looks a lot better. I'll try to go over the text later tonight and get back to you.
Comment #14
Crell CreditAttribution: Crell commentedOops, cross-posted.
Comment #15
vitezslav.smid CreditAttribution: vitezslav.smid commentedAnother try. According to http://drupal.org/node/1354 submit and validation handlers shouldn't be @ingroup forms, so I removed these comments. Insteads, the handlers are referenced by @see from their form builders (the formatting conventions again).
Applying the patch Works For MeTM.
Comment #16
drewish CreditAttribution: drewish commentedvitezslav.smid, i haven't tried re-applying it yet but make sure you do a "cvs up" before your roll the patch.
Comment #17
JirkaRybka CreditAttribution: JirkaRybka commentedNo need for cvs - I always succeeded patching against the latest dev tarball. But it seems that vitezslav.smid is patching against the latest beta (beta4), which is a few days old now. Please download the 6.x-dev version (and refresh before each new attempt, it's contents changes daily) - that's the newest code all developers are working on. Beta releases are only for testing by wider audience, not refreshed after release anymore. http://drupal.org/node/97368
Comment #18
vitezslav.smid CreditAttribution: vitezslav.smid commentedA patch against 6.x-dev. There were, of course, many changes to the code since beta4. I have edited the patch manually to keep just my changes to the documentation. I have little experience with diffs and patching so I don't know if this is the right way. Applying works for me, however.
Comment #19
JirkaRybka CreditAttribution: JirkaRybka commentedPatch now applies cleanly, fine in this aspect :) (BTW - if needed to refresh later, just apply to new 6.x-dev, check/edit any failed bits only (offset doesn't hurt, fuzz is mostly just a reminder to double check that bit), and then re-create using diff - that's how I do it usually).
Now to the patch itself: It's big, and I read through it only just briefly. The texts in there looked good as far as I see, but again I only reviewed carefully less than 30%. Needs another set of eyes.
I found a problem though - there seems to be still some junk from the manual re-roll just done (i.e. some things unrelated to the docs update, most probably generated from the difference between beta4 and -dev and unnoticed):
- The patch removes system_update_6041() function inside system.install - obviously shouldn't do that.
- At the top of system.module, there's the version number defined - the patch have it from beta4 still. Shouldn't touch that.
- To be checked: there are some changed lines, which are seemingly identical (not changed at all) - not sure whether we're undoing some of the previously done whitespace-cleanups here? I spotted such a line inside system_block(), the top $Id line on system.install, and maybe missed some more. Would be nice to check, why we change these.
Otherwise this is a great piece of work, I have to say. Leaving as CNR in hope that someone take a closer look at the texts.
Comment #20
vitezslav.smid CreditAttribution: vitezslav.smid commentedI've fixed the errors mentioned by JirkaRybka but the patch is a failure. Three hunks keep failing whatever I do. I've been working on it for hours and I'm getting desperate now. No idea what to do.
Comment #21
JirkaRybka CreditAttribution: JirkaRybka commentedSeems like a patch edited by hand, some indentation on unchanged lines didn't match, $Id line skipped in system.module hunk - but still 3 hunks failed whatever I do. Patches should not be edited manually (perhaps I'm wrong, but it seems like it here). Re-rolling is easily done as follows (my personal practice, note that nearly everyone have own beliefs, and also CVS is really popular here):
- Download latest 6.x-dev, unpack and rename the folder
- put the old patch somewhere else
- inside the unpacked and renamed folder apply the patch
patch -p0 < path/filename.patch
- see which files/hunks failed
- correct the failures manually (go to these files, see the rejected bits and original state now saved in extra files, and re-create the missing changes in files)
- unpack a fresh copy of 6.x-dev from the downloaded archive aside
- roll a brand new patch file by doing a diff - I generally use inside the renamed/edited folder
diff -urp --strip-trailing-cr ../drupal-6.x-dev ./ > ../filename.patch
which grabs the fresh 6.x-dev from parent folder and puts the new patch there, and ignores any Windows-style line endings (which are often cause of invisible problems).- check whether the new patch is OK (contains everything, but nothing unwanted).
Now I attempted to re-roll it for you, but I leave the last point (check if it's OK) to you. It's getting really late night here.
Also there was still the '6.0-beta4' version in the patch - I fixed that.
Further review needed.
Comment #22
vitezslav.smid CreditAttribution: vitezslav.smid commentedThank you, you have saved my life here. The patch needed just a few tiny changes. It applies cleanly for me. You are right, I did edit my previous patch manually - and I said I didn't know if it is the right way. Well, now I know better :). Thanks again!
The latest version which needs review is attached. By the way, somebody else has been working on the documentation lately. Several functions have comments which weren't there when I started working on the issue. Mostly they have the same meaning as mine, only rephrased.
Comment #23
aclight CreditAttribution: aclight commentedI tried applying the patch to D6 head and it did apply (though with some fuzz and offset). The Doxygen looks fine for the most part, but I didn't take the time to read through each docblock to check the validity of the code comments.
Comment #24
webchickI went and gave this a thorough look-over. *Vast* improvements in here, and I actually learned some new things about functions we have in system.module that I never knew about. :)
Still applies with some off-sets. I'm going to mark this sucker RTBC and give you credit for the patch. *Awesome* job!!
Comment #25
webchickOh, btw. Could you please cross-post your final patch back to http://code.google.com/p/google-highly-open-participation-drupal/issues/... so it's on the "official" Google tracker?
Comment #26
Gábor HojtsyHm, I looked into the patch, and found some stuff, which I did not like:
Hm, we don't name what we return ($comments).
These newlines look very odd.
Ugh? It is really not used?
Replaced an @ingroup themeable with a @themeable??
This looks like in need of a little bit of a cleanup still.
Comment #27
vitezslav.smid CreditAttribution: vitezslav.smid commentedAttached is a new version of the patch. Made against the newest -dev version.
I wouldn't do somthing like this intentionally. Mistakes of this kind appeared here and there and I was actually fixing them. This one escaped my attention. Fixed.
mēa culpa. Fixed.
If we are talking about comment_form() then yes, the variable is not referenced inside the function body. Or so it seems to me, please correct me if I am wrong.
No idea how this could have happened, I apologize. Fixed.
Comment #28
vitezslav.smid CreditAttribution: vitezslav.smid commentedComment #29
Gábor HojtsyGood, I quickly looked over the patch, and did not identify any obvious problems introduced by this patch (I did not check validity of comments one by one, as this was done above), so committed, thanks.
Comment #30
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.