Files: 
CommentFileSizeAuthor
#12 doc-cleanup-contextual-module-1332648-6.patch2.27 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,980 pass(es).
[ View ]
#10 doc-cleanup-contextual-module-1332648-5.patch2.29 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,987 pass(es).
[ View ]
#8 doc-cleanup-contextual-module-1332648-4.patch1.06 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,963 pass(es).
[ View ]
#7 doc-cleanup-contextual-module-1332648-3.patch31.21 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,830 pass(es).
[ View ]
#3 doc-cleanup-contextual-module-1332648-2.patch1.06 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,840 pass(es).
[ View ]
#1 doc-cleanup-contextual-module-1332648.patch1.06 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,811 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 33,811 pass(es).
[ View ]

That was a quick one.

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.

StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 33,840 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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.

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

Status:Needs work» Needs review
StatusFileSize
new31.21 KB
PASSED: [[SimpleTest]]: [MySQL] 33,830 pass(es).
[ View ]

This patch just adds the "the".

StatusFileSize
new1.06 KB
PASSED: [[SimpleTest]]: [MySQL] 33,963 pass(es).
[ View ]

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

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

StatusFileSize
new2.29 KB
PASSED: [[SimpleTest]]: [MySQL] 33,987 pass(es).
[ View ]

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.

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.

StatusFileSize
new2.27 KB
PASSED: [[SimpleTest]]: [MySQL] 33,980 pass(es).
[ View ]

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.

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 ?

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Looks good.

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?

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).

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

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.