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

Allow auto-fixing Generic.Commenting.DocComment #3752

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Feb 3, 2023

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 the master branch easily on PHP 8.2.

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.

@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');
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@fredden
Copy link
Contributor Author

fredden commented Feb 3, 2023

Thanks for the feedback. I'll respond to each block in the coming days.

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.

None yet

2 participants