Add missing type hinting to Color module docblocks

.

This is a first attempt at this issue. Mainly worked on /Drupal/color/Tests/ColorTest.php and a little in color.module.

@yanniboi: please follow the following standards:

1. @param lines should not have empty lines between them
2. @param comment should be on the same line as parameter type and name

Thank you!

Rerolled and removed line between @params.

Not entirely sure what you mean by:

@param comment should be on the same line as parameter type and name

I have been trying to follow the instructions here [#1312352], if I am still wrong, feel free to correct.

@yanniboi: I've reread instructions from [#1312352], please ignore my remark about @param description being on the same line.
Patch looks good for me now

I'm not 100% sure of the standard here, so moving to "documentation" so jhodgdon can verify.

1. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -21,8 +21,25 @@ class ColorTest extends WebTestBase {
   +   * @var object  protected $big_user;
   

   No ; at the end.
   Also, I don't believe "protected" should be here either.

2. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -21,8 +21,25 @@ class ColorTest extends WebTestBase {
   +   * @var array
   

   Missing $themes.

3. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -21,8 +21,25 @@ class ColorTest extends WebTestBase {
   +   * @var array
   

   Missing $colorTests.

Found a couple other things, here's a new patch.

Assigning back.

1. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -17,14 +17,34 @@ class ColorTest extends WebTestBase {
   +   * @var array $modules
   ...
   +   * @var object $big_user
   ...
   +   * @var array $themes
   ...
   +   * @var array $colorTests
   

   We don't include variable names in @var/@return, only @param

2. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -17,14 +17,34 @@ class ColorTest extends WebTestBase {
   +  /**
   +   * Provide test information.
   +   */
   

   No docblock on getInfo()

3. +++ b/core/modules/color/lib/Drupal/color/Tests/ColorTest.php
   @@ -33,6 +53,9 @@ public static function getInfo() {
      }
   +  /**
   

   Missing blank line

Addressed #10.1 and #10.2. #10.3 is actually a dreditor issue, it's ignoring lines that don't start with +/- and are "empty".

Removed $modules

The standards in this patch are OK, but the documentation is a bit questionable I think:

a) In Drupal 8, is this even accurate?

+  /**
+   * A user with permission to administer themes.
+   *
+   * @var object
+   */
   protected $big_user;

Shouldn't this be referencing a User class or interface?

b) Then the next chunk:

+
+  /**
+   * The theme to be tested.
+   *
+   * @var array
+   */
   protected $themes;

The docs say it is "the theme to be tested" but the @var says it is an array and the variable name is $themes, so I don't think the docs are accurate?

c)

+
+  /**
+   * Colors whose validity will be tested.
+   *
+   * @var array
+   */
   protected $colorTests;

So this is an array of colors? I don't think so. The colors are not even the array values, as you can see from this code that sets up this array:

    $this->colorTests = array(
      '#000' => TRUE,
      '#123456' => TRUE,
      '#abcdef' => TRUE,
...

A bit more explanation would be good.

d) And I'm guessing that

+   * @param array $test_values
+   *   An associative array of values to be tested.
    */
   function _testColor($theme, $test_values) {

refers to the same type of array? "array of values to be tested" tells me really nothing.

e) Are we even doing inheritdoc on setUp() methods yet? I don't think that coding standard has been updated, and meanwhile, I think inheritdoc is supposed to be omitted?

Taking a stab at cleaning up the documentation.

Trying again.

Thanks! This is getting closer.

A few things still to fix:

a) Please do not use @var object for Users objects. See
https://drupal.org/coding-standards/docs#types
It needs to be an interface type.

b)

   /**
    * Tests the Color module functionality using the given theme.
+   *
+   * @param string $theme
+   *   The machine name of the theme being tested.
+   * @param array $test_values
+   *   An associative array of test settings for themes.
    */
   function _testColor($theme, $test_values) {

The $test_values documentation... what kind of settings anyway? Saying "test settings for themes" doesn't really tell me anything.

c) In general, when saying something is an associative array, it's very helpful to explain what the keys and values are in the documentation. Just saying it's an array doesn't really document it very well. I would still need to go read the code to figure out what it is.

I've make re-rolled patch with corrections above.
@jhodgdon, I am only doubt about point c). I took a look at other tests and I didn't see there explanation array structure of the variable or class property. Is it nessesary?

I would prefer that you either:

a) Add good documentation that actually explains what is going on.

or

b) If there is a function that you don't want to document or don't know how to document, simply do not make any changes to that function's documentation in your patch. Leave it for someone else to do later or leave it undocumented.

The thing is... programmers can always read the code to find out what is going on. They will read documentation if it is there, but if the documentation is incorrect or lacks useful information, it is just a waste of their time to have that documentation there, because they will read it and either be mislead or still just have to read the code if they want to know what is happening.

Thanks!

It sounds like the line in question is this one?

An array of valid and invalid color values.

What if we change it to:

Associative array of hex color strings to test. Keys are the color string and values are a boolean set to TRUE for valid colors.

Added changes from previos comment for the variable $colorTests:

Removed trailing spaces from the comment for the variable $colorTests.

I think new description makes much more sense.

Our docs standards require that the first line of any doc block be a one-line one-sentence description, followed by a blank line. So this is not OK:

+  /**
+   * Associative array of hex color strings to test. Keys are the color string
+   * and values are a boolean set to TRUE for valid colors.
+   *
+   * @var array
+   */
   protected $colorTests;

Please separate after the first sentence.

Also, "Boolean" should technically be capitalized when it is used in a sentence, as opposed to "bool" when used as a data type in @var or @param or @return.

Thanks!

Adding tag.

Separated sentences in the comment and fixed the typo of name.

Looks good!

+   * Keys are the color string and values are a Boolean set to TRUE for valid colors.

This line exceeds 80 characters, can you please wrap it?

Yes, it's done.

Thanks! That works. Committed to 8.x.