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.

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

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

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

Conversation

jlherren
Copy link

Add an option to EmptyStatementSniff to allow a comment in an otherwise empty block to be considered non-empty.

@jlherren
Copy link
Author

Is there a way to document the new option in EmptyStatementStandard.xml?

Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

@jlherren I've had a quick look at the changes and left some thoughts.

Is there a way to document the new option in EmptyStatementStandard.xml?

All custom properties for the PHPCS native sniffs are listed in the wiki. This one would need to be added on merge as well.

*
* @var string[]
*/
public $allowComment = [];
Copy link
Contributor

@jrfnl jrfnl Aug 11, 2021

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.

@jrfnl
Copy link
Contributor

jrfnl commented Jul 13, 2023

While doing some PR triage I suddenly realized this is a (nearly exact) duplicate of #2240

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants