Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sven.lauer’s picture

Status: Active » Needs review
FileSize
1.06 KB

That was a quick one.

jhodgdon’s picture

Status: Needs review » Needs work

Looks OK... one small thing to fix up:

+ * @file
+ * Attaches behaviors for contextual module.

When referring to a module, either say "Contextual module" or "contextual.module". The latter is somewhat preferred, as it becomes a link on api.drupal.org.

sven.lauer’s picture

Rerolling with a fix.

BTW, is this documented somewhere. I've seen the "contextual.module"-type of reference, but I thought this is kind of misleading, given that what one wants to reference is the module itself, not just the .module file.

sven.lauer’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

True about contextual.module referring to the module file itself. In that case, capitalize and use "the": "the Contextual module".

This needs a newline after the @file docblock:

+/**
+ * @file
+ * Attaches behaviors for Contextual module.
+ */
+
 (function ($) {

And the word "the", and we're good to go.

jhodgdon’s picture

Regarding where the convention for modules is documented.... We enforced this convention extensively during the D7 cycle when we overhauled the core modules' hook_help(), and I'm sure we put it in the instructions for how to write a hook_help() (wherever that is...), but it didn't make it to the general writing style guide. So I just added it:
http://drupal.org/style-guide/content#moduleref
Sorry about that!

Oh, here's the standard for hook_help that we wrote, which includes (but does not explicitly state) this standard (the example is from the node module, but we don't really explain the standard):
http://drupal.org/node/632280

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
31.21 KB

This patch just adds the "the".

sven.lauer’s picture

Ups, forgot to do a rebase there. Here is the right patch.

xjm’s picture

It's a relief to see a short module. :) Looked it over and noticed a few things:

sven.lauer’s picture

Agreed on the pre-render callback. I also added an @see contextual_element_info().

Of course, ultimately, the pre-render callback signature should be documented in a single place, and then be removed from this function

I also was not aware that CSS files get docblocks, so I added those.

xjm’s picture

Status: Needs review » Needs work
+++ b/core/modules/contextual/contextual-rtl.cssundefined
@@ -1,4 +1,8 @@
-
+/**
+ * @file
+ * Stylesheet specific to right-to-left languages.
+ */
+ ¶

There's one bit of trailing whitespace here. Other than that, I think this patch is ready.

sven.lauer’s picture

Darn. I guess I should really add the "kill all trailing whitespace" rule in my editor to js and css files.

Re-rolled version just fixes the whitespace issue in the rtl.css file.

sven.lauer’s picture

A note, looking back over this issue (though it has come up in others, as well): The de facto standard of having a blank line after the @file docblock is not documented, I think. Maybe we should add it to http://drupal.org/node/1354#files ?

sven.lauer’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

jhodgdon’s picture

RE #13 - if you do not leave a blank line after the @file docblock, it becomes the docblock for the next thing in the file, which in this case is an (undocumented) variable or function. I don't really think we need to add that to the standards page?

sven.lauer’s picture

Hm, okay, so this is not a standard but a fact about the Doxygen implementation / syntax ... which I did not know about (I would have supposed that Doxygen processes the @file directive and hence knows that this applies to the whole file).

http://drupal.org/node/1354 does explain general Doxygen syntax and "pure standards" (I.e. "this is the way we do things"---like the blank line between @param and @return). So it might be a good idea to document this somewhere (Though then again, I suppose it does not come up terribly often outside of cleanup sprints).

jhodgdon’s picture

OK. I edited node/1354 to make this hopefully a little clearer (both the general section at the top and the file section).

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs backport to D7

Looks good, seems like this could be backported to D7 as well, minus that "Pre-render callback:" bit.

So, committed and pushed to 8.x (in full) and 7.x (with that hunk reverted).

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