-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Allow auto-fixing Generic.Commenting.DocComment #3752
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
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
See also #3751 where this same change is being applied to the
4.0branch.Note that I did development on the
4.0branch for this and cherry-picked the resulting commit here. I couldn't get the tests to pass with themasterbranch easily on PHP 8.2.