PEAR/FunctionComment: bug fix - handling of blank lines in pre-amble #830
Conversation
The `PEAR.Commenting.FunctionComment` sniff intends to flag blank lines between a function docblock and the function declaration.
However, as things are, there are - IMO - two bugs in the logic for this:
Given a code block which looks like this:
```php
class HandleBlankLinesBetweenDocblockAndDeclarationWithAttributes
{
/**
* Blank line between docblock and attribute.
*
* @return mixed
*/
#[ReturnTypeWillChange]
#[
AnotherAttribute
]#[AndAThirdAsWell]
public function blankLineDetectionA()
{
}//end blankLineDetectionA()
}
```
There will be only one error and it will read:
```
[x] There must be no blank lines after the function comment (PEAR.Commenting.FunctionComment.SpacingAfter)
```
This is confusing as the blank line may not be after the function comment, but after an attribute instead.
Additionally, the sniff also flags blank lines _within_ attributes, while that is outside of the purview of the sniff. (Should be handled by an attribute specific sniff)
What I would expect would be for the sniff to:
a) Throw a separate error for each (set of) blank lines found.
b) For the error message to more accurately reflect what is being flagged.
> Note: while in PHPCS this gets confusing, the fixer already fixes this correctly.
This commit changes the `SpacingAfter` error to comply with the above expectations
Includes test, though there are also some pre-existing tests which show this issue and for which the behaviour is changed.
_Note: while it will still be messy, it may be easier to review this PR while ignoring whitespace changes._
| if ($tokens[$i]['code'] === T_WHITESPACE | ||
| && $tokens[$i]['line'] !== $tokens[($i + 1)]['line'] | ||
| if ($tokens[$i]['column'] !== 1 | ||
| || $tokens[$i]['code'] !== T_WHITESPACE |
There was a problem hiding this comment.
Have you considered adding a test where this condition is true and $tokens[$i]['column'] is false? As far as I can check, in the current tests whenever $tokens[$i]['code'] !== T_WHITESPACE is true, $tokens[$i]['column'] !== 1 is also true. Thus the former is never executed.
There was a problem hiding this comment.
No, I did not, as those are pre-existing conditions which I've not changed. I've just grouped them into one if - continue as having multiple didn't make much sense.
Writing tests for pre-existing code is outside the scope of this PR.
There was a problem hiding this comment.
That is fair.
I asked because this condition was inverted. There are test cases that cover the first token of a line between the comment end and the function declaration being T_WHITESPACE, but there is no test case for it not being T_WHITESPACE.
There was a problem hiding this comment.
They are inverted because the flow has changed from "complies with condition -> throw error" to "does not comply with condition -> continue without error".
That is not a functional change. The behaviour of the sniff with regards to those conditions is unchanged - and outside the scope of this PR.
There was a problem hiding this comment.
I'm not saying the sniff couldn't use more tests, most sniffs can. Those additional tests though, do not belong with the functional change this PR is making, so should be added in a separate PR.
| || $tokens[$i]['code'] !== T_WHITESPACE | ||
| || $tokens[$i]['line'] === $tokens[($i + 1)]['line'] | ||
| // Do not report blank lines after a PHPCS annotation as removing the blank lines could change the meaning. | ||
| || isset(Tokens::$phpcsCommentTokens[$tokens[($i - 1)]['code']]) === true |
There was a problem hiding this comment.
Similar to the above, have you considered adding a test where isset(Tokens::$phpcsCommentTokens[$tokens[($i - 1)]['code']]) === true is true?
Description
The
PEAR.Commenting.FunctionCommentsniff intends to flag blank lines between a function docblock and the function declaration.However, as things are, there are - IMO - two bugs in the logic for this:
Given a code block which looks like this:
There will be only one error and it will read:
This is confusing as the blank line may not be after the function comment, but after an attribute instead.
Additionally, the sniff also flags blank lines within attributes, while that is outside of the purview of the sniff. (Should be handled by an attribute specific sniff)
What I would expect, would be for the sniff to:
a) Throw a separate error for each (set of) blank lines found.
b) For the error message to more accurately reflect what is being flagged.
This commit changes the
SpacingAftererror to comply with the above expectationsIncludes test, though there are also some pre-existing tests which show this issue and for which the behaviour is changed.
Note: while it will still be messy, it may be easier to review this PR while ignoring whitespace changes.
Suggested changelog entry
PEAR.Commenting.FunctionComment: improved message for "blank lines between docblock and declaration" check.
PEAR.Commenting.FunctionComment will no longer remove blank lines within attributes.
Related issues/external references
Loosely related to #152
Types of changes