Problem/Motivation

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

Proposed resolution

Correct the following in includes files beginning with H-M:

  • Ensure that each file has an @file docblock.
  • 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.

Remaining tasks

@param and @return formatting

  • install.inc (some indentation is incorrect).
  • menu.inc (missing punctuation; @return that are actually updates to variables passed by reference).

Optional parameter documentation

  • image.inc
  • install.core.inc
  • install.inc
  • lock.inc
  • mail.inc
  • menu.inc
  • module.inc

List formatting

  • image.inc

Missing @param

  • install.core.inc
  • mail.inc
  • menu.inc
  • module.inc

Missing @return

  • install.core.inc
  • install.inc
  • lock.inc
  • mail.inc
  • menu.inc
  • module.inc

Miscellaneous

  • install_configure_form() does a lot more than construct a FAPI array.
  • $tag = !$tag; in drupal_html_to_text(). No inline comment. Yeah.
  • Node-specific code in menu.inc. :(
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Title: Clean up API docs for includes directory, files starting with I-M » Clean up API docs for includes directory, files starting with H-M
Status: Active » Needs work
FileSize
32.47 KB

Cleanup for character limit, lines above @return, and @see/@ingroup positioning. No fixes to summaries or file docblocks yet. Waiting on #1315992: No standard for documenting menu callbacks and perhaps #1317826: Provide general documentation standard for callbacks.

xjm’s picture

Issue tags: -Needs backport to D7

Untagging.

Lars Toomre’s picture

In reviewing this patch, I note that there are indentation problems with some functions. For instance, in drupal_install_mkdir()

  * @param $message
  *  (optional) Whether to output messages. Defaults to TRUE.
+ *
  * @return
  *  TRUE/FALSE whether or not the directory was successfully created.

each of the explanations needs to be indented one additional space. Same goes for st() function.

I am aware that type hinting of parameters and return values is not required (but recommended). However, I think the documentation for both of these functions would be more clear if type hinting was included (e.g. '@param bool $message' and '@return bool').

In language_negotiation_get(),

+ *   Optional. By default retrieves values from the 'language_types' variable
+ *   to avoid unnecessary hook invocations.
+ *   If set to FALSE retrieves values from the actual language type
+ *   definitions. This allows to react to alterations performed on the
+ *   definitions by modules installed after the 'language_types' variable is
+ *   set.

the sting 'Optional.' shorld be '(optional)'. I also would add a comma after FALSE.

xjm’s picture

Again... the patch is still marked "Needs Work..." and we are not fixing everything about the docblocks, just the targeted things. And the previous patch is getting thrown out. :)

xjm’s picture

Status: Needs work » Postponed

Postponing for core directory change.

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Agreed - we're not generally putting types into these patches -- it makes the patch *much* harder to review, since you need to go read the function and verify the types are correct.

xjm’s picture

Status: Postponed » Active

Next!

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

Tagging. Also, xjm do you still plan to do this one?

xjm’s picture

Yep, just waiting on #1317620: Clean up API docs for includes directory, files starting with D-G and then I will do this one.

xjm’s picture

FileSize
431 bytes

We're (nearly) under thresholds now so here goes.

The only missing @file docblock.

xjm’s picture

Status: Active » Needs work
FileSize
885 bytes
1.29 KB

The only misplaced @see.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Blank lines above @return.

Note that a lot of install.inc has bad indentation for @param/@return (one space rather than two). Added that to the summary as a followup.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
2.56 KB
6.29 KB

Function summaries and rewrapping in image.inc.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Lots of form constructor fun and followups needed for install.core.inc.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
841 bytes
21.56 KB
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
1.34 KB
22.89 KB
xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
3.21 KB
24.77 KB

And this is enough for today. menu.inc and module.inc remaining.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
18.59 KB
43.08 KB

Not so bad.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.81 KB
47.9 KB

Smaller than the last two, for sure.

xjm’s picture

Issue summary: View changes

.

xjm’s picture

To help review this patch:

  1. Read the scope of the issue as defined in the issue summary above.
  2. Pick an interdiff above.
  3. Review only the changed lines (beginning with a + or -, not the additional lines that are provided for context.
  4. Ensure that the text in the added lines is clear, grammatically correct English, with capitalization, periods, etc. Keep an eye out for typos.
  5. Ensure that the updated lines meet the specific standards applicable in each case:
  6. Apply the whole patch from #19 to your local repository. Check the relevant file(s) for your interdiff and ensure that there are no other cleanups needed in that category. (For #10 - #12, check in all includes with names beginning with D-G. For #13 - #19, also ensure that no lines in any docblock are over 80 characters unless they are within @code/@endcode or @link/@endlink.)
  7. Document what you reviewed and what you found specifically in your review comment.
sphism’s picture

Just had a look through #19 and here are some notes:

image.inc
In the function headings sometimes use 'Returns...', sometimes use 'Gets...' and sometimes use 'Retrieves...'

Since all 3 of those functions have _get in them i suggest we use 'Gets...'

2 new lines after:
function image_desaturate(stdClass $image) {
return image_toolkit_invoke('desaturate', $image);
}

Crops an image to the rectangle specified by the given rectangle.
-- sounds odd, perhaps:
Crops an image to a rectangle specified by the given dimensions.

no comments > 80 chars
can't see any other issues with doc blocks, except all missing param and return types, but that's for another sprint right?

sphism’s picture

interdiff-image.txt from #13 looks good to except minor issues mentioned in #21

sphism’s picture

regarding interdiff-module.txt:

Determines which modules require and are required by each module.
--- sounds odd, is an improvement on "Find dependencies any level deep and fill in required by information too." but still not ideal

Other than that the interdiff looks good.

-----

Checking the applied patch:

No comments are over 80 chars

2 line breaks after function module_load_all

This seems to be written differently to all the others:
/**
* Array of modules required by core.
*/
function drupal_required_modules() {
--- Wouldn't it be "Returns an array of modules required by core." or something?

Yep all looks good, that's some nicely formatted and commented code right there :)

kbasarab’s picture

+ * Determines which modules require and are required by each module.

Should read differently. Doesn't quite sound right. Maybe:
"Determines which modules are required by each module"

In the full patch:
+ * Returns the router path, or the path for of a default local task's parent.

Should possibly read:
"* Returns the router path, or the path for a default local task's parent."

xjm’s picture

@kbasarab reviewed interdiff-module.txt.

+ * Determines which modules require and are required by each module.

Should read differently. Doesn't quite sound right. Maybe:
"Determines which modules are required by each module"

I disagree with this particular point. The sentence is grammatically correct, and the revised suggestion does not cover the full functionality. The other change looks correct, though. Thanks @kbasarab and @sphism!

Remaining interdiffs to review: #10, #11, #12, #14, #15, #16, #17, and #18.

kbasarab’s picture

#10 Looks good. Not much in that one.
#11 Looks good.

#12:

@@ -815,6 +827,7 @@ function drupal_requirements_severity(&$requirements) {
  *
  * @param $module
  *   Machine name of module to check.
+ *
  * @return
  *   TRUE/FALSE depending on the requirements are in place.
  */

Should TRUE/FALSE statement read:
"TRUE/FALSE depending on if the requirements are in place."

#14, #15, #16, #17 and #18 look good to me. Another set of eyes probably wouldn't be bad though.

xjm’s picture

Actually, I believe it should read:

TRUE or FALSE, depending on whether the requirements are met.

Thanks @kbasarab!

xjm’s picture

#1464944: Installation of configuration system fails in some cases added another couple incorrect docblocks here.

jhodgdon’s picture

Status: Needs review » Needs work

It looks like a lot of this patch has been reviewed, and I think there were some review comments that need to be addressed. Can we get those fixed, and then I would be happy (or at least willing) to give this another review?

jhodgdon’s picture

Assigned: xjm » Unassigned

I'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
46.68 KB
47.16 KB

Re-worked @xjm's patch from #19 to apply cleanly, then implemented the above suggestions. New patch, revised #21 patch, and interdiff attached.

jhodgdon’s picture

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

This looks great! Committed to 8.x.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
44.76 KB

First pass at a D7 backport.

jhodgdon’s picture

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

The last submitted patch, h-m_complete-1317626-33.patch, failed testing.

jhodgdon’s picture

Issue tags: +Needs reroll

Looks like this one needs a reroll... and I'm *trying* to get through these patches to get them reviewed/committed... sorry for delays, I had a busy summer!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
44.75 KB

No worries, re-rolled!

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Active

Everything in this patch looks like an improvement, thanks! Committed to 7.x.

So... I took a look at the H-M include files, and I think we could go back to 8.x (and 7.x port) and fix a few more issues:
- menu.inc - _menu_check_access() - first line doesn't end in "."
- menu.inc - template_preprocess_menu_tree() - first line isn't documented the way we normally do these now: Implements template_preprocess_HOOK() for theme_menu_tree()
- menu.inc - theme_menu_local_tasks() doesn't have $variable documentation
- mail.inc - drupal_wrap_mail() has no @return
- install.inc - install_verify_config_directory() has a two-line start instead of one-line, and a wrapping issue in the next paragraph
- install.core.inc - install_select_language_form() needs @param $files docs
- None of the form constructors in install.core.inc has @ingroup forms in docs
- install.core.inc - _install_configure_form() should be documented as a form constructor

Albert Volkman’s picture

Status: Active » Needs review
FileSize
4.39 KB

Resolves issues found by @jhodgdon.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! That's mostly great... A couple of corrections/additions:

a) (install.core.inc)

+ *
+ * @param $files
+ *   An array of .po files.
+ *
+ * @see install_find_translations()
+ * @ingroup forms
  */
 function install_select_language_form($form, &$form_state, $files) {

Looking at how $files is used in the function, I see:

  foreach ($files as $file) {
    if (isset($standard_languages[$file->langcode])) {
...

and it's called from install_select_languages(), which passes in the output of install_find_translations()...

b) Related: It looks like we need a @return for install_find_translations() that explains what the return value is... which can be taken from install_find_translation_files(). (install.core.inc)

c) _install_configure_form() still needs @param $install_state (install.core.inc)

d) Looking at the code in theme_menu_local_tasks() in menu.inc, it looks like both of the variable elements are optional, so maybe that should be mentioned in the documentation?

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
5.15 KB

This is my attempt to resolve these issues. I _think_ I understood what you were saying in (a), but wasn't entirely sure.

Status: Needs review » Needs work

The last submitted patch, h-m_complete-1317626-41.patch, failed testing.

jhodgdon’s picture

Here's the syntax error in the patch:

+ */
+function install_find_translation_files($langcode = NULL) {
  */
 function install_select_language_form($form, &$form_state, $files) {

I think the new wording is good. :)

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
5.09 KB

Ah, got in a hurry. I was trying to get this patch together before my work day started :)

jhodgdon’s picture

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

Great, thanks! Committed to 8.x.

I think we can just port this to D7 and then call this issue fixed.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.51 KB

Backported.

jhodgdon’s picture

Status: Needs review » Needs work

It looks like in D7 we also need to fix this first line to conform with our usual Form standards:

/**
  * Form API array definition for language selection.
+ *
+ * @ingroup forms
  */
 function install_select_locale_form($form, &$form_state, $locales, $profilename) {

Other than that, the patch looks good to me, thanks!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
379 bytes
3.57 KB

Done!

jhodgdon’s picture

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

Thanks! I'll get this committed (probably tomorrow).

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Whew! committed this one too.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Lars Toomre’s picture

Glad to see this one committed. I realise that @jhodgon has said in the past that there is no need for an empty line before a @ingroup directive. However, for the sake of of consistency, I have been adding blank lines between @return, @throws, @see, @ingroup and @todo.

I am unclear why @see and @ingroup directives can/should be grouped together whereas the others are supposed to be separated by a blank line. Can you elaborate @jhodgdon so I do not create more patches that violate what was committed here? Thanks in advance.

jhodgdon’s picture

It is never a mistake to add the blank lines, but our standards do not insist upon it.

For instance if there is one @see and one @ingroup, it doesn't really add a lot of readability to have a blank line in there. If there are 5 @see lines and 3 @ingroup lines, then it's probably a good idea. There should always be a blank line after @return if there are @see or @ingroup though... Anyway, I guess we don't have a hard-and-fast rule for this one -- use your own judgment. I won't reject patches that have the blank lines, or that don't.

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

Anonymous’s picture

Issue summary: View changes

.

Andrew Answer’s picture