https://drupal.org/coding-standards/docs#order states

a blank line before the first parameter and a blank line after the last parameter before the @return section starts

, and gives an order for docblock elements.

The attached patch adds space mostly before @return elements, and shifts a few @see to other parts of the docblock according to the coding standards defined order.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deekayen’s picture

Title: Change spacing » Fix spacing and section order in Simpletest docblocks
jhodgdon’s picture

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

Super! Easy-to-review patch and it all looks good to me.

I'll get it committed when the test turns green (just in case). Thanks!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This patch does not apply today.

shnark’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
15.35 KB

There were a couple of conflicts, there were descriptions that were line wrapped in the latest version of core. Also, one of the @returns had a typehint. I resolved them.

dang42’s picture

I've done the following:

* Reviewed the API documentation requirements for docblocks
* Reviewed the patch in #4
* The changes made by the patch all comply with the API requirements and are all within the scope of the issue summary
* Applied the patch to a local install

Yet to complete:

* Review patched files to confirm that all non-compliant instances have been resolved

I should be able to get this done sometime tonight...

-------------------------
Note - in the following patched file:

core/modules/simpletest/lib/Drupal/simpletest/TestBase.php

there is some extra whitespace / lines (lines 591-593). This is outside the scope of the issue, but should be fixed...?

Note 2 - thank you very much to YesCT for helping me with my first issue cue review, and all the mentors at the Nerdery / 2013 TwinCities Drupal Camp!!!

dang42’s picture

TestBase.php:

Other than the whitespace issue noted in #5 above, I don't see any problems with either spacing or section order in the docblocks.

WebTestBase.php:

1 - a space needs to be added after line 2259
2 - a space needs to be removed between two @param tags at line 3363

Tests/SimpleTestTest.php:

No spacing or section order issues. However, I think I might have found another bit to be fixed (based on the API info found here).

Lines 247-251 of core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestTest.php:

   * @param string $message Assertion message.
   * @param string $type Assertion type.
   * @param string $status Assertion status.
   * @param string $file File where the assertion originated.
   * @param string $functuion Function where the assertion originated.

The syntax notes for the @param tag state that

The @param tag is followed by an optional data type indicator, then the variable name of the parameter, and then a newline. The following paragraph is considered by the API module to be documentation.

Is this technically a "spacing" issue? Is it an issue at all? If it is a problem, and it is appropriate to fix it here, I'll be happy to do so.

--------------------

I am a complete noob at this issue que stuff and didn't get a chance to learn today (well, yesterday...) at the sprint how to create a patch. I'll do my best to figure it out on my own and put it up here in the next couple of days - and if I haven't figured it out by the time the D8 core mentoring starts on Wednesday, I'll pop in there to ask for help... :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This patch is limited to making sure there is a blank line between @param and @return, and that the sections are in the right order, as stated in the issue summary. There are many other documentation standards issues in these files, unfortunately, but the patches get really big if we try to fix them all at once and they're hard to review.

Anyway, thanks for the review! It sounds like within the scope of this issue, this is RTBC according to narragansett, so I'll change the status and see about committing it shortly.

dang42’s picture

Got it - thanks for clarifying that for me!

Given that clarification and stripping out all the off-topic issues I mentioned, there are still two fixes to be made to one of the files included in the patch shown in #4:

WebTestBase.php:

1 - a space needs to be added after line 2259 to separate the @param & @return tags
2 - a space needs to be removed between two @param tags at line 3363

Everything else should be good to go...

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

As per #8, two small changes need to be made, so updating status accordingly. Thanks!

dang42’s picture

Status: Needs work » Needs review
FileSize
15.88 KB

So, after a little bit of 'core mentoring' from "valthebald" (thank you very much!), I did the following:

* made the necessary changes to WebTestBase.php
* ran 'git diff' to get the correct output for those two changes
* manually copied the output into the correct spots in the patch file from #4 above
* tested the new patch to be sure it still works - and it does...
* renamed it to add_simpletest_docblock_spacing_fix_order_10.patch and attached it here

I don't know how attribution works on this kind of thing, so to be clear:

The patch uploaded here is essentially EllaTheHarpy's - I only added two more changes, I did not remove anything. This is all Ella's work, with a couple of additions from me... :)

If I need to do anything else, or do something differently, just let me know and I'll be glad to make it happen!

jhodgdon’s picture

We try to give credit to everyone who contributed to a patch. :)

The only other thing that would be helpful here is if you could also provide an "interdiff" along with your patch.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

But don't bother with an interdiff now -- I just reviewed the patch in #10 and it all looks good. Thanks! I'll get it committed after the bot turns green.

dang42’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
847 bytes

Ask and ye shall receive...

ETA - I created & uploaded it before I saw your second reply.

That's alright - I needed to figure out how to do it for future reference anyway... ;)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Yes, good practice. :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5a66f5d and pushed to 8.x. Thanks!

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