PHPDoc rules (and not Doxygen as one would assume)

subscribe

OK, I'll kick off the brainstorming, because this is indeed frustrating, and one of the things most effective at breaking the spirits of fledgling core contributors, IMO.

Sometimes things do get committed, with a "Needs Docs" flag added after the fact. Perhaps we can make this policy, and take an agile philosophy on the initial docs: Only do what is absolutely minimally acceptable. Less is more, because we can always add more later. Don't let best be the enemy of good enough. Insert cliché here. They're clichés because they're true.

We can even require 'committed, needs docs' issues to stay at 'critical' status to ensure they are addressed before release. Now the bikeshedders are responsible for delaying the entire release, so perhaps they'll relax their nitpicking for the sake of making delivery.

Or perhaps a policy along the lines of: If you mark a patch "Needs Work" due to docs you must supply a patch that, at a minimum, merely removes the offending docs. The reviewer is then forced to ask, "Despite imperfect docs, is this patch better with nothing?" (Sometimes it is.) Or, "Is ANY documentation better than NO documentation?" (Usually it is.) Then, the patch can be marked "Needs Review" if the developer feels the removed documentation was not strictly necessary, or revise as needed. Granted, this 'remove by default' strategy works less well for adding a completely new API, but at least sheds light on what's worth complaining about.

To put it bluntly, if you won't take the time to delete what you don't like, you haven't earned the right to complain. A barrier to bikeshedding, if nothing else.

Also, tests provide the best documentation anyway, so given that tests are required, less prose should be needed. As long as tests follow coding standards, and whatever docs do exists use correct doxygen syntax, other documentation standards should not prevent working code from being committed, within reason.

To put another light on it: It is my understanding that the super-stringent coding standards are mainly required so that code is able to be parsed by meta-tools (e.g., coder module, etc). This does not apply to documentation in the same way; the doxygen standard ensures that docs can be parsed to the extent we need, so nothing beyond that need be strictly enforced.

Lastly, the relatively recent addition of user comments to api.drupal.org mitigates the need to get documentation 'perfect' right away.

This could come come up with any subsystem maintainer or any of the teams we have such as the docs and usability team, or the more informal, scalability/performance, database, etc. The current pattern seems to resemble a linear progression through a single issue passing responsibility for patches back and forth between different teams or individuals with different immediate goals. To my mind, and obvious solution is to parallelize the process, and allow each team to continue their work independent of the current progress of another team. Could you imagine docs being written before the code itself? ;-)

While I hate to punt issues to solutions that are coming soon, but not yet in sight, it seems to me that this would be much easier with distributed version control, as you wouldn't spend time merging patches if someone was working on docs as the same time as the implementation, and you wouldn't have to wait for the other team to finish/post your patch before starting on yours.

Also related but without a delivery date, is #569552: Provide a mechanism for issue meta discussions which would possibly allow docs and implementation to be split into separate issues, even though I am a little wary of that. It may even be possible to use a Blocks status or requires status to indicate that documentation needs to be finished before committing the original issue.

@ #2 I don't want to speak for others in this thread, but I really doubt anyone is questioning the quality of value of our docs, and relaxing the standards. There are a million reasons why Drupal got to where it is, but docs are part of that, and I don't think that we should compromise the quality of the docs.

I don't think the root issue is even related to the problem of folks complaining but not being willing to write a patch. Its more about the process and back and forth of code and documentation refinement and the effects that each has on the other.

Tests do not replace the type of docs that we are discussing here, which are inline docs, mostly the docs that appear on api.drupal.org. Code style is a separate issue and has nothing to do with coder module (which now uses the Grammer Parser). Much of the docs syntax is important for getting picked up by the API module though, which does use Grammar Parser, but that doesn't necessarily break down the content contained within a docblock itself.

I disagree with several points in #2 (and I should mention that I'm probably the reason chx filed this issue). I'm obviously passionate about Drupal documentation... A few thoughts:

- I don't think any patch should be committed with inaccurate or incomplete documentation, or with no documentation. The standards we have for doxygen comments and in-code comments, in my opinion, are just as important as the other coding standards we have, and I also don't think we should commit patches that violate our coding standards. The patches marked "needs work / needs doc" don't get addressed, in practice, and I don't think it's a good idea to have that kind of code/doc in our code base -- anything that is committed could get used as an example for someone else writing code/doc.

- Tests do not constitute adequate documentation, when compared to a clear exposition on what a function does, the parameters it takes, and the return value. That is why we have documentation standards. Relatively few of the functions in the Drupal source code are tested directly in such a way that you could read the tests and figure out everything the function is supposed to do, what all the parameters are, etc. I mean, you could just as well say we shouldn't have any doxygen headers, because someone could just read the code of the function and figure it out.

- I'm somewhat less concerned about grammar/spelling/writing style issues in patches, as opposed to general clarity and accuracy, if they don't stand in the way of people being able to undersand the doc. Obviously, I prefer not to have these problems in the doc, but they are minor compared to the many "this doc is wrong" issues that are now in the documentation component issue queue.

- I also think that you can't just say that anyone who criticizes a patch for its documentation should have to write a new one. Do you apply that same standard to code criticism? I don't think so. People comment on patches and mark them "needs work" all the time without creating a new patch that fixes the problems they identify, and actually if they do create a new patch, then they can't be the reviewer of their own new patch, and it can hold up the review process.

I never suggested anyone RTBC or commit inaccurate docs. If something is blatantly wrong, remove it. If correct information is necessary, it will be added.

Tests do not constitute adequate documentation, when compared to a clear exposition

What's "clear exposition" ? That's entirely subjective. Tests, however, are objective. Either they pass, or they don't, and anyone can copy and modify them as desired to learn how the code works, or use as a template for their own work.

And most of the published documentation standards ( http://drupal.org/node/161085 http://drupal.org/coding-standards#comment am I missing any?) have nothing to do with clarity. They're merely mechanical.

There are a million reasons why Drupal got to where it is, but docs are part of that

True, and I'm not saying that we should reduce our documentation efforts as compared to Drupal 6 and prior. But one of those million reasons is also the tradition of answering questions such as "How do I user some_api()?" with "Look at how core module X does it."

Learning by actual trial and error beats the worlds "clearest" prose any day of the week. Inline comments best serve when trial and error may be insufficient -- e.g., when its possible to make it 'work' in a way that is insecure or performs poorly.

Oh yes, we have other standards:
http://drupal.org/node/1354 (main doxygen syntax guide, some mechanical, some syntax)
http://drupal.org/node/338208 (Handbook style/usage guide, also applicable in most cases to doxygen comments)

Perfect. Here's a concrete proposal:

- Compliance with http://drupal.org/node/1354 is required for RTBC, though not necessarily implementation of every directive. You can leave it out, but you can't do it wrong.

- Compliance with http://drupal.org/node/338208 is recommended for code comments, but it is not a blocker for RTBC. Issues based on these standard can be resolved in follow-up patches. Matters of grammatical error should be marked 'critical' (release blockers); matters of convenience and convention may be permitted as 'normal.'

- Documentation that is technically incorrect justifies CNW. Documentation that is merely grammatically non-standard can be addressed in post-commit follow-up.

-Documentation that describes HOW to use an API function should be accompanied by a demonstrative SimpleTest and/or example module, in lieu of extensive prose, with a @see directive identifying the demonstrated use in the doxygen block above the documented API.

What do we think?

I think the proposal in #8 is almost there. A couple of thoughts (obviously from the point of view of the API doc maintainer, who's fighting an uphill battle to improve the API doc):

a) I think if documentation leaves out components (e.g. doesn't describe params or return value), then the patch shouldn't be committed. So, I would just say that compliance with node/1354 is required for RTBC. I don't really think it is too much to ask. I mean, for sure, leave essays in the long description and usage examples for later patches, but every function should have at a minimum a compliant doxygen header (a one-liner description, @param description for all its params, and @return if it has a return value). I don't think leaving out any part of that is really OK. Or am I missing something -- can you point to guidelines/directives in node/1354 that you think are optional and can be left out?

b) I would also clarify that node/338208 is a guideline mainly for the doxygen comments. I don't personally think in-code comments need to be as strict on grammar/style, as long as they are clear. But the doxygen comments are used to generate api.drupal.org, so I think they need to adhere to the higher standard.

c) I'm OK with the idea that node/338208 (style/grammar/usage/etc.) is not a blocker for RTBC, if it's just a few small wording changes here and there that are needed and the issue is marked back to "needs work / needs documentation (tag)" when it's committed. The rare patch whose doc grammar is so bad as to be unreadable or unclear due to wording should of course be CNW, right?

d) You didn't include http://drupal.org/coding-standards#comment -- my opinion is that any code that doesn't follow our coding standards should not be committed, and that includes if the in-code comments do not follow the in-code comment standards.

Sorry, cross post...

I don't think the 3rd person verb part is probably automatable by Coder, but most of the rest of the standards in
http://drupal.org/node/1354 probably are.

@ #11

a) Agreed. Requiring one-line description, @params, and @return is reasonable. Everything else is optional, or at least, not a RTBC blocker.

b) Good clarification.

c) "The rare patch whose doc grammar is so bad as to be unreadable or unclear due to wording should of course be CNW, right?" Only if it's a required thing in (a). Otherwise, decide if it's better deleted and submit a patch, or just live with it, or submit a patch with fixes so it's CNR.

d) I guess I was thinking that goes without saying, since it's explicitly labeled a 'coding standard' for 'non-documentation' and not a 'documentation standard.' We're really just discussing doxygen/documentation here.

RE #13 on verbs: I have quite often recently seen people do things like this:

Reads the foo and return a bar.

I.e., the first verb is 3rd person and 2nd verb isn't, and Coder couldn't catch that.

But of course you are quite right: making sure the first word ends in S would probably catch 80% of the verb tense problems at least, and have few false problem reports.

Subscribing.

Ok, putting in my 2cents.

I don't think most people don't leave comments because of the reasons chx outlined. Most people understand the code they write and thus don't need to write comments for themselves - they would only leave comments for others. If you're rushing to try to get a patch in at the last minute or even if you just want to finish up a patch and get it over with, you can skimp on docs because they aren't necessary to the function of the patch and aren't mandated as heavily as tests are.

Code style isn't maintained for ease of use of automated tools. The reason we maintain such strict coding standards is so that everyone who looks at the code knows exactly what to expect, and because it's easy to follow the format when writing new code. I think it's for this reason that we should maintain documentation as well. The reason is twofold - (1) we should maintain excellent documentation so that anyone looking at the code will be able to understand it and also be able to know what to expect and *how* to understand it, and (2) we should maintain excellent documentation so that those who can/want to/will write more documentation (to facilitate part 1) can do so easily, because they will already know exactly what the documentation is supposed to look like.

I'm not so sure I agree. While this may be your reasoning and I can understand why that would be the case, I'm not sure that this is the reason for the majority of developers. For me, it's certainly not that way - I based my summary above on what I do.

I'm not even really talking about automated rules - just the need for them to be, in general, enforced. It's easier for me to write docs if I know exactly what they should look like.

Not that it matters though. We both agree that docs are good, we need them, and automation is good, we need it too.

There are a fair amount of comment rules in Coder Review already, including proper @see directive checking and comment indentions (from #9). Some of the other comment reviews (3rd person verbs, etc) are going to be very hard to check (see #826148: Extra strict rules?). Coder Review uses regular expressions, not Grammer Parser (which Coder Upgrade uses), so it's a bit harder to check complex patterns.

There's also http://drupal.org/project/coder_tough_love , which provides additional Doxygen style checking.

I'll my personal feeling about the current state of things.

First of all, many functions are either badly documented or even still have old and thus erroneous documentation. (maybe from 4.7?) Okay, I did not check 7.x as much, but in 6.x, that was still the case. In any event, it is always very difficult to maintain documentation. I know, I like to work on it.

Second, and I think that would be the best part, the current state of the documentation does not parse properly with Doxygen. We get TONS of errors. Many missing parameters, missing $ sign on variables, the @param does not specify whether it is an in, out or in,out (as in @param[in,out] $form This is the form array), most of us will also write stuff like <foo> tag without putting a backslash in front of the < which will make <foo> a tag in the output instead of showing inline....

Most of those errors can be checked by running Doxygen and returning an error if Doxygen outputs one or more warning (In most cases, Doxygen does not generate errors. It is a friendly app.) I'm not too sure that Coder can support that though. The tests should do that each time a patch is offered. And the SVN (or are we moving to git?! Yack!) repository could be checked at the time it generates the tarballs. If an error occurs, flag it, at least.

That would be my take. 8-)

I had loads of problems with C++ to generate Warning free documentations from Doxygen. But with PHP, there is always an easy way to fix those problems.

Thank you.
Alexis

Alexis: Despite the name, Drupal's documentation format is not Doxygen. It's a fork of Doxygen, influenced by PHPDoc, with our own custom additions and our own custom parser (api.module). To be quite frank, calling it Doxygen at this point is disingenuous. It's not going to parse properly with the "native" Doxygen parser.

Hopefully by Drupal 8 we can migrate to pure PHPDoc, like the rest of the PHP world uses, but that's TBD.

Drupal's API docs do improve with each version, but there is certainly room for improvement still. The best way to improve them is to file issues to address issues you find and submit patches. Documentation pages are readily accepted the same as code patches.

@Crell,

Hence the issue title. 8-)

I thought I read in the Drupal docs that you were using Doxygen. Now, Doxygen or PHPDoc, all the same. If you run your api.module or PHPDoc and get any error, then refuse the patch, just like with code. Actually, running Coder against patches could save us a lot of time (only problem today with the test engine is that the output is not always very talkative...)

> Documentation pages are readily accepted the same as code patches.

Right... That's why only a couple of the patches I submitted passed? And that after several months... 8-)

I even asked for a fix in the Comments module that breaks the comments tree for any PostgreSQL user. It was found since 4.7 and it still isn't fixed. All I'm asking there is to apply a quick patch so at least PostgreSQL do not lose data. Since 2006. "readily". I like it. LOL.

Alexis

I said they're readily accepted the same as code patches. Code patches frequently languish for longer than we want, too. :-) That's a manpower issue. It affects comment and code patches equally, unfortunately. :-(

In any case, back to the point of this thread, having coder be able to spot issues in comments ++ Now let's get back on topic.

Bump.

The docblock rules that would be good to have in Coder, and which are not there now:

- Max 80-character lines
- Blank line before @return

Tagging

These issues have been filed elsewhere in the Coder project so I'm closing this as a duplicate at this point. I'm also just filed a new meta issue to deal with having more checks in Coder for documentation:
#1924890: [meta] Documentation standards rules