Part of meta-issue #1310084: [meta] API documentation cleanup sprint

Correct the following for the overlay module:

  • Ensure that all files have an @file docblock that conforms to standards.
  • Ensure that all @return lines are preceded by a blank line.
  • Ensure that @see and @ingroup lines are always at the end of docblocks.
  • Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
  • Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use
    the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
  • Make incidental style and grammar corrections only to those docblocks already being updated.
Files: 
CommentFileSizeAuthor
#31 overlay-doc-cleanup-1398404-31.patch7.75 KBdcam
PASSED: [[SimpleTest]]: [MySQL] 39,563 pass(es).
[ View ]
#27 doc-cleanup-overlay-module-1398404-26.patch2.88 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 42,093 pass(es).
[ View ]
#22 doc-cleanup-overlay-module-1398404-22.patch6.6 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 42,124 pass(es).
[ View ]
#22 interdiff.txt1.28 KBbatigolix
#20 doc-cleanup-overlay-module-1398404-20.patch6.63 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 40,568 pass(es).
[ View ]
#16 doc-cleanup-overlay-module-1398404-16.patch6.78 KBbatigolix
PASSED: [[SimpleTest]]: [MySQL] 37,214 pass(es).
[ View ]
#11 doc-cleanup-overlay-module-1398404-11.patch8.01 KBbatigolix
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-cleanup-overlay-module-1398404-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 doc-cleanup-overlay-module-1398404-8.patch6.41 KBNROTC_Webmaster
PASSED: [[SimpleTest]]: [MySQL] 35,361 pass(es).
[ View ]
#8 overlay-interdiff.txt7.45 KBNROTC_Webmaster
#5 drupal8.file-docblocks.1398404.5.patch1.76 KBaLearner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.file-docblocks.1398404.5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

synth3tk’s picture

Title:Clean up API docs for [module or directory]» Clean up API docs for overlay module

Fixed issue name.

synth3tk’s picture

Assigned:synth3tk» Unassigned

Ran out of time before basic training. Up for grabs.

jhodgdon’s picture

Tagging

aLearner’s picture

Assigned:Unassigned» aLearner

I'm happy to help.

aLearner’s picture

Issue summary:View changes

Updated description with more details.

aLearner’s picture

StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.file-docblocks.1398404.5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I attempted to ensure that all files have an @file docblock that conforms to standards.

aLearner’s picture

Status:Active» Needs review

Setting the status to 'needs to review'.

xjm’s picture

Status:Needs review» Needs work

Thanks @aLearner! Patch review:

+++ b/core/modules/overlay/overlay-child-rtl.cssundefined
@@ -1,4 +1,9 @@
+/**
+ * @file
+ * This is a JavaScript file that's titled 'overlay-child-rtl.css'
+ */

This is actually a CSS file, not a JavaScript file. However, it would be better to instead describe the file's purpose. In this case, I'd suggest:
RTL styling for Overlay child pages.

Let's also change the other docblocks similarly to describe the files' purposes. (If you aren't sure what to put, try looking at examples from other core modules.)

+++ b/core/modules/overlay/overlay-child.cssundefined
@@ -1,4 +1,9 @@
+ */
+ ¶

At the end of each docblock, you have a character of trailing whitespace. This will need to be removed. I suggest configuring your editor to display the trailing whitespace so it doesn't get added accidentally.

Once you've made those changes, you can create a new patch, like we did earlier today. (Make an additional commit on your overlay branch, and then do git diff 8.x to get the patch.) Then, you can submit your updated patch at Needs Review again, and if it looks good we'll move on to the next bullet point. Good luck!

NROTC_Webmaster’s picture

Status:Needs work» Needs review
StatusFileSize
new7.45 KB
new6.41 KB
PASSED: [[SimpleTest]]: [MySQL] 35,361 pass(es).
[ View ]

aLearner,

I'm uploading a new patch that includes some todo's. I didn't go through all of the files in depth but rather glanced through them. I hope this is useful, but xjm/jhodgdon are the experts in this area.

interdiff from #7.

NROTC_Webmaster’s picture

Status:Needs review» Needs work
  1. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -300,6 +300,8 @@ function overlay_page_alter(&$page) {
    /**
      * Menu callback; dismisses the overlay accessibility message for this user.
    + *
    + * @todo - Update this to current standards.
  2. This should follow http://drupal.org/node/1354#menu-callback

  3. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -383,6 +387,8 @@ function overlay_disable_message() {
      * Returns the HTML for the message about how to disable the overlay.
      *
      * @see overlay_disable_message()
    + *
    + * @todo - Update this to current standards.
      */
    function theme_overlay_disable_message($variables) {
  4. I think this should follow http://drupal.org/node/1354#themeable or see http://drupal.org/node/1354#hookimpl for additional guidance.

  5. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -473,6 +479,8 @@ function overlay_preprocess_maintenance_page(&$variables) {
      * Preprocesses template variables for overlay.tpl.php
      *
      * @see overlay.tpl.php
    + *
    + * @todo - Update this to current standards.
      */
    function template_preprocess_overlay(&$variables) {
  6. I think this should follow http://drupal.org/node/1354#hookimpl

The rest of the @todo's in this document should be similar to the ones listed above.

batigolix’s picture

Assigned:aLearner» batigolix

will give this a shot

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new8.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-cleanup-overlay-module-1398404-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

the attached patch implements the comments from #11

Status:Needs review» Needs work

The last submitted patch, doc-cleanup-overlay-module-1398404-11.patch, failed testing.

batigolix’s picture

cannot reroll the patch from #8

jhodgdon’s picture

What problem are you having rerolling the patch? See
http://drupal.org/patch/reroll

And if you no longer plan to work on this issue, please go ahead and un-assign it. Thanks for trying!

jhodgdon’s picture

Assigned:batigolix» Unassigned
batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new6.78 KB
PASSED: [[SimpleTest]]: [MySQL] 37,214 pass(es).
[ View ]

not going to give up, yet.

batigolix’s picture

Assigned:Unassigned» batigolix

re-assiging this to myself

jhodgdon’s picture

jhodgdon’s picture

Status:Needs review» Needs work

Apologies again for the delay! I'm finally getting back to reviewing this... It mostly looks really good! I only found:

a)

+++ b/core/modules/overlay/overlay.module
@@ -322,7 +322,7 @@ function overlay_user_dismiss_message_access() {
}

/**
- * Menu callback; dismisses the overlay accessibility message for this user.
+ * Menu callback; Dismisses the overlay accessibility message for this user.

That ; should be a :

b)

+ * @param $variables
+ *   An associative array containing:
+ *   - profile_link: The link to this user's account
+ *   - dismiss_message_link: Link to dismiss the overlay
+ *
+ * @ingroup themeable
  */
function theme_overlay_disable_message($variables) {

These two list items need to end in ".". Also I think the second one should also start with "The"?... Hm. Actually, I don't think this is right. Looking at the code, it looks like $variables only has $variables['element']. Those two array items are part of $element. So it should probably say that the only array item is element, and the documentation would probably be 'A render element for the message'... however this type of render element isn't documented anywhere or even declared in a hook_element_info as far as I can tell, so I think it's rather bogus the way this theme function is set up... So probably it would be best to do something like:

An associative array with an 'element' element, which itself is an associative array containing:

(and then have the two items you had in there).

c)

/**
- * Preprocesses template variables for overlay.tpl.php
+ * Implements hook_preprocess_HOOK() for overlay.tpl.php
  *
- * @see overlay.tpl.php
+ * If the current page request is inside the overlay, add appropriate classes
+ * to the <body> element, and simplify the page title.
  */
function template_preprocess_overlay(&$variables) {

Based on the name of this function, it's actually implementing template_preprocess_HOOK(), not hook_preprocess_HOOK().

d)

+ *
+ * @todo - Update this to current standards.
  */
function template_process_overlay(&$variables) {

I guess this and the other @todo sections still need to be done?

e)

+++ b/core/modules/overlay/overlay.tpl.php
@@ -11,8 +11,8 @@
  *   (e.g., the view and edit tabs when displaying a node).
  *
  * Helper variables:
- * - $classes_array: Array of html class attribute values. It is flattened
- *   into a string within the variable $classes.
+ * - $classes_array: Array of html class attribute values. It is flattened into
+ *   a string within the variable $classes.
  *

html -> HTML

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new6.63 KB
PASSED: [[SimpleTest]]: [MySQL] 40,568 pass(es).
[ View ]

Remarks a) - e) from #19 were incorporated in this new patch

jhodgdon’s picture

Status:Needs review» Needs work

This is mostly great! A couple of minor fixes and we'll be done here:

a)

/**
- * Processes variables for overlay.tpl.php
+ * Implements template_process_HOOK() for overlay.tpl.php
  *
- * @see template_preprocess_overlay()
- * @see overlay.tpl.php
+ * Places the rendered HTML for the page body into a top level variable.
  */
function template_process_overlay(&$variables) {
-  // Place the rendered HTML for the page body into a top level variable.
+  //
   $variables['page'] = $variables['page']['#children'];
}

I agree with moving the comment into the documentation block, but rather than leaving an empty comment behind in the function body, just remove the line completely.

b) Also I don't see why the @see to the preprocess function was removed? That looks useful, and maybe a @see in the reverse direction should be added to the preprocess function?

c)

/**
- * Delivery callback to display an empty page.
+ * Callback to display an empty page.

I'm not sure what function this is documenting, but it should probably follow the standards of
http://drupal.org/node/1354#functions or
http://drupal.org/node/1354#callbacks or
http://drupal.org/node/1354#menu_callback
whichever is most appropriate. As it is, it doesn't seem to follow any standard.

batigolix’s picture

Status:Needs work» Needs review
StatusFileSize
new1.28 KB
new6.6 KB
PASSED: [[SimpleTest]]: [MySQL] 42,124 pass(es).
[ View ]

attached patch incorporates the comments from #21

as for comment c) : I'm not sure what this documentation should look like. It seems an a-typical function and I don't know what example to follow

jhodgdon’s picture

jhodgdon’s picture

Status:Needs review» Active

OMG this patch still applies -- committed to 8.x. THANKS!

Now for a bit of followup that is still needed:

overlay.module
- overlay_user_dismiss_message_access() - non-standard and too long first line -- see
http://drupal.org/node/1354#menu-callback
for standard -- it's a hook_menu() access callback.
- overlay_user_dismiss_message() -- same standard, it's a page callback.
- overlay_display_empty_page() - needs different first line... looking at the @return (and the way the function is defined and used), I think something like "Stores and returns whether an empty page override is needed" would be better?
- overlay_close_dialog() -- also needs a different first line. How about just "Requests that the overlay..." (taking out the "Callback to request..."?
- overlay_request_refresh() - verb tense first line
- overlay_request_page_refresh() - same
- overlay_trigger_refresh() - same

batigolix’s picture

I'll see if I can dedicate some time for this in the next days, so leave this assigned to me.

If there is no action within a week, feel free to assign this to someone else

batigolix’s picture

Status:Active» Needs review
StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 42,093 pass(es).
[ View ]

patch incorporates comments from #24

Status:Needs review» Needs work
Issue tags:-Novice, -needs backport to D7, -docs-cleanup-2011

The last submitted patch, doc-cleanup-overlay-module-1398404-26.patch, failed testing.

jhodgdon’s picture

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7, +docs-cleanup-2011
jhodgdon’s picture

Version:8.x-dev» 7.x-dev
Status:Needs review» Patch (to be ported)

Thanks! Committed to 8.x. I think we're ready to port both the patch in #27 and #22 to 7.x now (they can be combined).

dcam’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new7.75 KB
PASSED: [[SimpleTest]]: [MySQL] 39,563 pass(es).
[ View ]

Backported #22 and #27 to D7.

jhodgdon’s picture

Assigned:batigolix» jhodgdon
Status:Needs review» Reviewed & tested by the community

Thanks, looks good! I'll get it committed shortly.

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Thanks again to all who worked on this! This is now committed to 7.x.

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.