This one style issue has always driven me crazy, so here is a patch, finally, to remove it.

In English usage, a nominalization is a verb turned into a noun. Generally is obscures clarity and should be avoided.

All throughout core, we have:

/**
 * Implementation of hook_foo().
 */

The docblock standard is a short, active sentence. Such as "Retrieve a blocked IP address from the database." (Taken from blocked_ip_load()).

This patch changes _all_ instances of "Implementation of hook_foo()" to "Implement hook_foo()" in one pass.

CommentFileSizeAuthor
#9 implement_no_of.patch169.85 KBstella
implement_no_of.patch167.95 KBagentrickard
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

agentrickard’s picture

Component: base system » documentation
Issue tags: +Coding standards

Tag.

EclipseGc’s picture

Can't say this bothers me one way or another, but the patch looks good, and I appreciate anyone trying to make our coding standards better.

Eclipse

stella’s picture

Looks good to me, +1

catch’s picture

Status: Needs review » Reviewed & tested by the community

Works for me too. Will be hard to re-learn but not a reason to leave as is.

Crell’s picture

Status: Reviewed & tested by the community » Needs review

Does this actually match the recommended verb structure for the rest of are docblocks? There are some verbage standards that require a noun phrase and others that require a verb phrase. I don't actually know what our standard is, if any.

agentrickard’s picture

Yes.

The first line of the block should contain a brief description of what the function does, beginning with a verb in the form "Do such and such" (rather than "Does such and such").

http://drupal.org/node/1354 -- Documenting Functions.

The standard for documenting hook implementations (right below) contradicts this verb rule.

stella’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
FileSize
169.85 KB

Patch re-roll

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this ;)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation

Let's get this documented in the coding standards, please!

stella’s picture

Status: Needs work » Fixed
stella’s picture

Issue tags: -Needs documentation
jhodgdon’s picture

I think the standard mentioned in #6 is wrong, and filed a separate issue on this -- see #487802: Function doc standard for Drupal is non-standard in industry

agentrickard’s picture

Agreed. But this patch is still better than before. So this stays 'fixed.'

sun’s picture

Priority: Minor » Normal
Status: Fixed » Active

Sorry, but this was plain wrong.

Implement hook_foo(). sounds like and actually is a command.

Implement does not work as a verb turned into a noun. At least not in this context.

Implementation of hook_foo() is an exception to our documentation standards, because it does not document or summarize the function. It is rather a human-readable equivalent of @ingroup hook_implementations.

If we want to remove this exception and adhere to the current documentation standards, the proper change is to appropriately convert all those summaries into summaries that document the actual function body.

That would mean:

/**
 * Implementation of hook_node_view().
 */

becomes

/**
 * Attach comments to rendered node.
 *
 * @ingroup hook_implementations
 */

Please revert this patch. And do it properly (if at all).

agentrickard’s picture

No.

catch’s picture

I don't think we can do meaningful one line summaries for a lot of hooks - menu block_$op form_alter etc. Not to mention that'd break grepping for hook implementations.

agentrickard’s picture

Status: Active » Fixed

My point is that rolling this back doesn't improve anything. If you want one-line summaries, that is a separate issue.

jhodgdon’s picture

Sun (#17 above): Please check out and comment on #487802: Function doc standard for Drupal is non-standard in industry. Would love some support there!

sun’s picture

Status: Fixed » Active

The point is that rolling this back reverts to something that makes sense when reading it.

"Implement hook_foo()." sounds like a todo. A reminder to implement hook_foo().

jhodgdon’s picture

Sun: I totally agree with you. If we can get agreement on #487802: Function doc standard for Drupal is non-standard in industry, then my plan is at least to change them to "implements", and fix the other 2nd-person doc headers (they are all over the code). At the moment, however, our doc style guidelines say that 2nd person is the standard. It is not a good standard, in my opinion, but "Implement hook_foo()" is in compliance with the standard. Hence issue 487802. Please comment there...

Crell’s picture

Status: Active » Fixed
sun’s picture

Issue tags: +Hall of shame
Crell’s picture

Issue tags: -Hall of shame

Status: Fixed » Closed (fixed)
Issue tags: -Coding standards

Automatically closed -- issue fixed for 2 weeks with no activity.