-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow auto-fixing Generic.Commenting.DocComment #3752
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fredden Thanks for this PR.
I've not done a full review, but I have left some comments for you to think over based on a first glance at the PR.
Hope it helps.
@@ -71,9 +71,31 @@ public function process(File $phpcsFile, $stackPtr) | |||
if ($short === false) { | |||
// No content at all. | |||
$error = 'Doc comment is empty'; | |||
$phpcsFile->addError($error, $stackPtr, 'Empty'); | |||
$fix = $phpcsFile->addFixableError($error, $stackPtr, 'Empty'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an opinionated fix, which - in my opinion - should not be included in PHPCS.
The Doc comment is empty
error can lead to two distinctly different fixes/outcomes:
- Either the docblock should be removed (current fix).
- Or the docblock should be filled out.
As those different outcomes are mutually exclusive and the second one cannot be automated (at least not for the descriptions), I do not think this is an error which is suitable for auto-fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole standard is opinionated. ;)
Yes, I agree that there are two valid ways to satisfy the rule, and only one of these are able to be automated. That's why I chose this particular avenue.
Shall I remove the auto-fixing of this particular rule from this pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I remove the auto-fixing of this particular rule from this pull request?
Not for me to say, I'm a contributor to this repo, just like you.
Thanks for the feedback. I'll respond to each block in the coming days. |
See also #3751 where this same change is being applied to the
4.0
branch.Note that I did development on the
4.0
branch for this and cherry-picked the resulting commit here. I couldn't get the tests to pass with themaster
branch easily on PHP 8.2.