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

This is for the system module, subdirectory tests, files and sub-directories starting with E through I.

CommentFileSizeAuthor
#4 drupal-api_docs_cleanup-1431670-3.patch8.53 KBCB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Clean up API docs for simpletest/tests, E-I » Clean up API docs for system/tests, E-I

Updating scope for #1299424: Allow one module per directory and move system tests to core/modules/system.

CB’s picture

CB
Location Sydney
CreditAttribution: CB commented
Assigned: Unassigned » CB

Assigning to myself.

CB’s picture

CB
Location Sydney
CreditAttribution: CB commented
Status: Active » Needs review

Uploading a small patch merely for feedback to ensure I am going about it all the right way. I keep referring to the notes here as well as the Doxygen formatting.

I just didnt want to continue working through this if I was doing something fundamentally incorrect. :)

Thanks!

CB’s picture

CB
Location Sydney
CreditAttribution: CB commented
FileSize
drupal-api_docs_cleanup-1431670-3.patch8.53 KB

Woops, and heres my patch.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work 
+++ b/core/modules/system/tests/modules/error_test/error_test.moduleundefined
@@ -34,7 +38,14 @@ function error_test_menu() {
+ *   A Boolean for updating determining whether to collect the errors. ¶

trailing whitespace

+++ b/core/modules/system/tests/modules/file_test/lib/Drupal/file_test/DummyStreamWrapper.phpundefined
@@ -15,23 +15,32 @@ use Drupal\Core\StreamWrapper\LocalStream;
+   * @return ¶

trailing whitespace

There are a TON of extra files that need to be checked in "lib/Drupal/System/Tests" they used to be part of the test folder. Maybe you can look at the E-I directories...

-16 days to next Drupal core point release.

Alan D.’s picture

Alan D. CreditAttribution: Alan D. commented 
+; Information added by drupal.org packaging script on 2012-07-17
+version = "8.x-dev"
+core = "8.x"
+project = "drupal"
+datestamp = "1342484253"
+

This stuff shouldn't be in the code afaik :)

Also, many editors have options to trim trailing white space on save, it is worth looking into rather than trying to manually parse the file for these.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks for making a start on this! In addition to the comments by others above, I found a few things to take note of before you start on the full patch:

a)

+++ b/core/modules/system/tests/modules/error_test/error_test.module
@@ -1,4 +1,8 @@
 <?php
+/**
+ * @file
+ * Generates errors for testing Drupals error handling

Drupals -> Drupal's.

b)

/**
- * Menu callback; generate warnings to test the error handler.
+ * Generates warnings to test the error handler.

Probably the "Menu callback" should not have been removed here (just reformatted slightly). See
http://drupal.org/node/1354#menu-callback

c)

+++ b/core/modules/system/tests/modules/file_test/file_test.module
@@ -46,7 +46,15 @@ function file_test_stream_wrappers() {
 }
 
 /**
- * Form to test file uploads.
+ * Creates a form to test file uploads.

See http://drupal.org/node/1354#forms -- and also the form submission function that comes after this does not have the right documentation header.

d)

/**
- * Get the arguments passed to invocation of a given hook since
+ * Gets the arguments passed to invocation of a given hook since
  * file_test_reset() was last called.

This is a two-line description of a function. It needs to be shortened to one 80-character line:
http://drupal.org/node/1354#functions

e)

+ * @param $file
+ * @param $errors
+ *   The errors to be returned
+ *
+ * @return
+ *   Errors
  */
 function file_test_validator(File $file, $errors) {

- First @param needs a description.
- Second @param and @return - descriptions need to end in ".". See
http://drupal.org/node/1354#functions
And this needs to be done elsewhere in the patch too.

f)

/**
-   * Override getExternalUrl().
+   * Override getExternalUrl() for testing purposes.

Override -> Overrides

CB’s picture

CB
Location Sydney
CreditAttribution: CB commented

Thanks for the feedback guys, much appreciated.

I'm curious though, especially regarding jhodgdon's comment about @param $file needing a description, a lot of these test functions and methods have dummy parameters, and this is one. Its not used for anything at all - is it ok to just leave it, or should I write something like 'dummy parameter' etc? Or do I need to trail back through, find all instances of that function call and work out what $file can be (even though it is unused?)

Whitespace was to do after with the perl command posted in the parent issue, but I have since enabled auto trim of whitespace in my editor, so that wont be an issue again.

Jeez, its hard sometimes to reduce function descriptions to a single line! :) But, I'll go through and update them more thoroughly.

Thanks again.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Regarding dummy parameters -- I'm assuming this would be because this function is implementing some interface or overriding some base class function? In which case you can entirely omit the method documentation by using the syntax shown here in the DatabaseStatementBase::execute() or fetchAssoc() method:
http://drupal.org/node/1354#classes
Otherwise, yes, the parameter needs a description, which could say something like "Not used in this function."

CB’s picture

CB
Location Sydney
CreditAttribution: CB commented

This hasn't been forgotten guys, just been insanely busy. Will get some more done this week.

CB’s picture

CB
Location Sydney
CreditAttribution: CB commented

I wont be able to continue on this, I've been busy with the accessibility issues for D8. I'll remove myself from the list.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Assigned: CB » Unassigned
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!