Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#31 | overlay-doc-cleanup-1398404-31.patch | 7.75 KB | dcam |
#27 | doc-cleanup-overlay-module-1398404-26.patch | 2.88 KB | batigolix |
#22 | doc-cleanup-overlay-module-1398404-22.patch | 6.6 KB | batigolix |
#22 | interdiff.txt | 1.28 KB | batigolix |
#20 | doc-cleanup-overlay-module-1398404-20.patch | 6.63 KB | batigolix |
Comments
Comment #1
synth3tk CreditAttribution: synth3tk commentedFixed issue name.
Comment #2
synth3tk CreditAttribution: synth3tk commentedRan out of time before basic training. Up for grabs.
Comment #3
jhodgdonTagging
Comment #4
aLearner CreditAttribution: aLearner commentedI'm happy to help.
Comment #4.0
aLearner CreditAttribution: aLearner commentedUpdated description with more details.
Comment #5
aLearner CreditAttribution: aLearner commentedI attempted to ensure that all files have an @file docblock that conforms to standards.
Comment #6
aLearner CreditAttribution: aLearner commentedSetting the status to 'needs to review'.
Comment #7
xjmThanks @aLearner! Patch review:
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.)
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!Comment #8
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedaLearner,
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.
Comment #9
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThis should follow http://drupal.org/node/1354#menu-callback
I think this should follow http://drupal.org/node/1354#themeable or see http://drupal.org/node/1354#hookimpl for additional guidance.
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.
Comment #10
batigolixwill give this a shot
Comment #11
batigolixthe attached patch implements the comments from #11
Comment #13
batigolixcannot reroll the patch from #8
Comment #14
jhodgdonWhat 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!
Comment #15
jhodgdonComment #16
batigolixnot going to give up, yet.
Comment #17
batigolixre-assiging this to myself
Comment #18
jhodgdon#16: doc-cleanup-overlay-module-1398404-16.patch queued for re-testing.
Comment #19
jhodgdonApologies again for the delay! I'm finally getting back to reviewing this... It mostly looks really good! I only found:
a)
That ; should be a :
b)
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:
(and then have the two items you had in there).
c)
Based on the name of this function, it's actually implementing template_preprocess_HOOK(), not hook_preprocess_HOOK().
d)
I guess this and the other @todo sections still need to be done?
e)
html -> HTML
Comment #20
batigolixRemarks a) - e) from #19 were incorporated in this new patch
Comment #21
jhodgdonThis is mostly great! A couple of minor fixes and we'll be done here:
a)
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)
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.
Comment #22
batigolixattached 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
Comment #23
jhodgdon#22: doc-cleanup-overlay-module-1398404-22.patch queued for re-testing.
Comment #24
jhodgdonOMG 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
Comment #25
batigolixI'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
Comment #27
batigolixpatch incorporates comments from #24
Comment #29
jhodgdon#27: doc-cleanup-overlay-module-1398404-26.patch queued for re-testing.
Comment #30
jhodgdonThanks! 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).
Comment #31
dcam CreditAttribution: dcam commentedBackported #22 and #27 to D7.
Comment #32
jhodgdonThanks, looks good! I'll get it committed shortly.
Comment #33
jhodgdonThanks again to all who worked on this! This is now committed to 7.x.
Comment #34.0
(not verified) CreditAttribution: commentedUpdated issue summary.