Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generic/EmptyStatementSniff: Allow comments to be considered non-empty #3409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Generic/EmptyStatementSniff: Allow comments to be considered non-empty #3409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/Standards/Generic/Sniffs/CodeAnalysis/EmptyStatementSniff.php
View file
Open in desktop
Expand Up @@ -30,6 +30,13 @@
class EmptyStatementSniff implements Sniff
{

/**
* Empty statements which should not be reported when there is a comment in the body.
*
* @var string[]
*/
public $allowComment = [];
Copy link
Contributor

@jrfnl jrfnl Aug 11, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. I had to look at the docblock to understand what this property does. Trying to think whether I can come up with a more self-explanatory name, but not sure whether any of the names I came up with are any better... $notEmptyOnComment, $skipForComment etc

Might be more in line with the general direction I see for other sniffs to handle this with differentiation in the error code instead.

What I mean by that is, that for completely empty control structure bodies, the error code would remain the same as it is now, but when a comment is detected, but no code, the error code would be changed to something like Generic.CodeAnalysis.EmptyStatement.DetectedElseifOnlyComment.

This would be a breaking change as rulesets which currently ignore Generic.CodeAnalysis.EmptyStatement.DetectedElseif, would now get notifications again under the new error code if there is a comment.

So all in all, not sure what would be best. I think @gsherwood needs to weight in.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed struggled a bit with the naming here. PhpStorm calls it "Comments count as content", but that doesn't make it clear that the variable should contain statement names.

I like the idea of separate error codes, that would allow using error severity for empty bodies and warning for comment-only bodies. However, I'll rely on maintainers to decide if there should be a BC break here.



/**
* Registers the tokens that this sniff wants to listen for.
Expand Down Expand Up @@ -75,10 +82,16 @@ public function process(File $phpcsFile, $stackPtr)
return;
}

if (in_array(strtolower($token['content']), $this->allowComment, true) === true) {
$skipTokens = T_WHITESPACE;
} else {
$skipTokens = Tokens::$emptyTokens;
}

$next = $phpcsFile->findNext(
Tokens::$emptyTokens,
$skipTokens,
($token['scope_opener'] + 1),
($token['scope_closer'] - 1),
$token['scope_closer'],
true
);

Expand Down
18 changes: 18 additions & 0 deletions src/Standards/Generic/Tests/CodeAnalysis/EmptyStatementUnitTest.inc
View file
Open in desktop
Expand Up @@ -72,3 +72,21 @@ try {
if (true) {} elseif (false) {}

match($foo) {};

// phpcs:set Generic.CodeAnalysis.EmptyStatement allowComment[] elseif,catch
jlherren marked this conversation as resolved.
Show resolved Hide resolved

if ($foo) {
// Should complain here
} elseif ($bar) {
// Should not complain here
} else {
// Should complain here
}

TRY {
// Should complain here
} CATCH (Exception $exception) {
// Should not complain here
}

// phpcs:set Generic.CodeAnalysis.EmptyStatement allowComment[]
3 changes: 3 additions & 0 deletions src/Standards/Generic/Tests/CodeAnalysis/EmptyStatementUnitTest.php
View file
Open in desktop
Expand Up @@ -40,6 +40,9 @@ public function getErrorList()
68 => 1,
72 => 2,
74 => 1,
78 => 1,
82 => 1,
86 => 1,
];

}//end getErrorList()
Expand Down