Doxygen clean-up

Indeed - @file and function comments in general are quite lacking.

Aye, I like me a proper clean-up. ;)

Here's a draft for some up-cleaning. Please comment away - some of the texts could be way better than what I've tossed together here.

Here's a re-roll. Apparently it wasn't applying very cleanly anymore.

I intend to commit some of the more obvious stuff, like missing punctuation, replacing /* with /**, and wrongly documented argument/parameter names. I'll make a patch here first though, for a(n easy) review.

And I just went to have a look at core's style for @file blocks, and they have a new line between the // $Id$ line and the Doxygen block, so we should probably do it like that as well. (And I originally did for pathauto.admin.inc too. Hm...)

Here's a variety of changes I feel are minor. Committing these will (hopefully) make the "real" changes easier to find in the patch, and thus (hopefully) easier to review on their own. If no one gives a thumbs up or thumbs down, I'll just slip it in.

Okay. The attached patch was committed.

Keeping open per this conversation:

Freso: So. How about this scenario:
Freso: I commit the "full" Doxygen patch now/in a bit, but keep the issue as active.
Freso: Then tomorrow, on your plane, you go through the Doxygen strings and elaborate, add to, and/or correct as you seem fit. And make a patch.
Freso: Then either commit that or upload the patch for discussion.
Freso: And then when that's checked in, we "fix" the issue. :)
Freso: (I like that plan, as that makes the TODO's not linger in the code for eternity, but still marks that there will be stuff there.)
Greg: that sounds good to me

Oh, and once Greg has completed the Doxygen string re-writing, it should probably be ported to D5 before being marked "fixed".

Okay, since I didn't want to wait around for Greg to post his updates to this issue, I've made a patch which fixes some errors and omissions from my previous patch (notably, I'd somehow completely overlooked pathauto_taxonomy.inc :/).

Here's my best guesses on these. There could always be more information, but does this seem good enough?

It also includes a few whitespace fixes...

I played a bit with the last patch. Updated patch is attached.

@mustafau: To be honest, I'd've rather had the comment corrections in another patch. This patch is for cleaning up the Doxygen, and while Doxygen is part of the comments, other comments aren't part of the Doxygen. So, IMHO, these should go into their own comment clean-up, especially when also considering how pervasive they are in this patch.

But, about the Doxygen...

• You're moving the @defgroup to pathauto.inc... why? The "pathauto" group should be defined in pathauto.module (the main file) per Drupal documentation.
• You have a "wether", which should be "whether" (or possibly just keep the "if the").
• You also seem to break some lines to make them way shorter than the ~80 character limit of the Drupal coding style (inherited from the PEAR manual, even if I believe it was formerly specified specifically... ?). Please don't do this.
• There are a few cases of "Implementation of hook_pathauto() for foo module", where you've removed the "for foo module" - why? It's a perfectly good description and helps differentiate all the different _pathauto()'s in the files.
• In pathauto.admin.inc, you've snuck in a few lowercased "pathauto"'s.

Apart form this, there are some good improvements over Greg's patch - but the above needs to be cleared before I'll want to have it committed. :)

Updated patch. I have eliminated all of your concerns.

@mustafau: I just gave it a quick look, and it looks good. :) There are a few things I'd like to have clarified though.

• Why are you changing the @defgroup pathauto? The current is how it's shown in the handbook page. I'm not saying you're wrong, as you are probably better versed in Doxygen syntax than I am. I'd just like to learn and make sure we're doing The Right Thing™. :)
• I like Main file for the Pathauto module, which automatically generates aliases for content. better than Used to automatically generate path aliases for content. for the @file description of pathauto.module, as it tells what the file is, instead of the overall purpose/function of the module as a whole - this latter is for the @defgroup pathauto to do, IMHO.
• That white-space clean-up in the @@ -133,7 +137,7 @@ hunk of pathauto.module - you could probably just remove the line altogether. It doesn't look like it makes any sense to have a blank line there.
• You haven't made an issue for the comment clean-up! That's a serious draw back to your patch. ;) (Just kidding. But seriously, I would like you to make such an issue for your clean-up of the comments. :))

(Yes, I like lists. So shoot me. (Get the pun? Well, I guess it is somewhat obscure... (Also, I am going to bed now.)))

• I looked at form.inc as an example for @defgroup declaration.
• It is obvious that pathauto.module is the "Main file for the Pathauto module". There is no need to mention it.
• Done.
• I am going to work on comments after this patch is commited.

Ad §1: Well, I liked the old bried description better, "Pathauto: Automatically generates aliases for content". "Pathauto functions" is too vague and assumes you know what Pathauto does in advance. It needs to describe both what it is (ie., that which makes up "Pathauto") and what it does (ie., what Pathauto does - automatically generating content aliases).
Ad §2: I don't mind you removing the "Main file for the Pathauto module" bit - what I object to is that you're now just saying what it does instead of what it is. pathauto.module doesn't do anything, but it is something. The functions inside pathauto.module do stuff, the file itself doesn't. (If you re-read my comment, you might just find that this is what I originally objected to, too.) (Also: The original first stated what it was: "the main file of Pathauto", and then used a run-on sentence that told what Pathauto did - not what pathauto.module did.)
Ad §4: Alrighty then. :)

ps. Oooh. We're almost there. Can you feel it? =)

1: I give up.
2: Examples from core:

// $Id: aggregator.module,v 1.374.2.1 2008/04/09 21:11:44 goba Exp $

/**
 * @file
 * Used to aggregate syndicated content (RSS, RDF, and Atom).
 */

// $Id: path.module,v 1.138.2.1 2008/04/09 21:11:48 goba Exp $

/**
 * @file
 * Enables users to rename URLs.
 */

Even core isn't always right. :) (Those two examples should have that description moved to their @defgroup, as they describe what the module does as a whole, not what the specific file is. Read on...)

Think about the values semantically. What are they supposed to describe. As I see it, @defgroup is supposed to describe the group "pathauto", which is the entire Pathauto module (this view is backed by the previously referenced handbook page). So this is what needs to be described. @file is supposed to describe a file, and a file doesn't do anything on its own. Calling/loading the file won't do anything, thus it doesn't make any sense to say that the file does something. It does make sense, however, to describe what it is and/or what it contains.

However, since you've obviously just dropped changing these texts, it should be good to go in as it is now. I'll just keep it open a tad longer to see if Mr. Knaddison has anything to say on this.

Also, thanks a lot for all your work on this, mustafau! It really is appreciated, even if I may seem an ungrateful bastard. :)

Ok, this one does not change @file declaration in pathauto.module.

Well, well. I don't think I can find anything else to put me greasy finger on. But as I said, keeping it open a tad longer to see whether Greg has anything to chime in with. :)

Since you feel this is an improvement on my work - this is fine by me.

Would go for #18.

Alright! Committed! :D

Now we just need to back-port this to the 5.x-2.x branch... :)

Thanks to the policy of keeping 5.x-2.x and 6.x-1.x as similar as possible, most of the above patches applied with some simple offset. :) I've attached a patch combining the 1st and 2nd batch for 6.x to apply to 5.x-2.x, as well as a few corrections that aren't applicable for or already fixed in D6.

And committed to DRUPAL-5--2/5.x-2.x as well. I guess this is fixed, then. :)