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

Allow auto-fixing Generic.Commenting.DocComment #3752

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

Allow auto-fixing Generic.Commenting.DocComment #3752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
30d1897
Allow auto-fixing Generic.Commenting.DocComment
fredden Feb 3, 2023
4673571
Remove use of null coalesce operator
fredden Feb 4, 2023
7c9ded4
Avoid setting default for variable
fredden Feb 5, 2023
df3b5c5
Avoid setting default variable
fredden Feb 5, 2023
ecbd443
Merge remote-tracking branch 'upstream/master' into auto-fix/doc-comment
fredden Feb 27, 2023
88ac112
Fix problem identified by broken unit test
fredden Mar 29, 2023
c0d95ed
Remove 'risky' fixer for {Short,Long}NotCapital
fredden Jun 17, 2023
73a1afd
phpcbf
fredden Jun 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 179 additions & 17 deletions src/Standards/Generic/Sniffs/Commenting/DocCommentSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


if ($fix === true) {
$phpcsFile->fixer->beginChangeset();
for ($i = $commentStart; $i <= $commentEnd; $i++) {
$phpcsFile->fixer->replaceToken($i, '');
}

if (isset($tokens[($commentStart - 1)]) === true && isset($tokens[($commentEnd + 1)]) === true) {
$tokenBefore = $tokens[($commentStart - 1)];
$tokenAfter = $tokens[($commentEnd + 1)];
if ($tokenBefore['code'] === T_WHITESPACE
&& $tokenBefore['content'] === $phpcsFile->eolChar
&& $tokenAfter['code'] === T_WHITESPACE
&& $tokenAfter['content'] === $phpcsFile->eolChar
) {
$phpcsFile->fixer->replaceToken(($commentStart - 1), '');
}
}

$phpcsFile->fixer->endChangeset();
}

return;
}
}//end if

// The first line of the comment should just be the /** code.
if ($tokens[$short]['line'] === $tokens[$stackPtr]['line']) {
Expand Down Expand Up @@ -193,6 +215,8 @@ public function process(File $phpcsFile, $stackPtr)
return;
}

$indent = str_repeat(' ', $tokens[$stackPtr]['column']);

$firstTag = $tokens[$commentStart]['comment_tags'][0];
$prev = $phpcsFile->findPrevious($empty, ($firstTag - 1), $stackPtr, true);
if ($tokens[$firstTag]['line'] !== ($tokens[$prev]['line'] + 2)
Expand All @@ -210,7 +234,6 @@ public function process(File $phpcsFile, $stackPtr)
$phpcsFile->fixer->replaceToken($i, '');
}

$indent = str_repeat(' ', $tokens[$stackPtr]['column']);
$phpcsFile->fixer->addContent($prev, $phpcsFile->eolChar.$indent.'*'.$phpcsFile->eolChar);
$phpcsFile->fixer->endChangeset();
}
Expand All @@ -222,6 +245,7 @@ public function process(File $phpcsFile, $stackPtr)
$tagGroups = [];
$groupid = 0;
$paramGroupid = null;
$firstTagAfterParams = null;
foreach ($tokens[$commentStart]['comment_tags'] as $pos => $tag) {
if ($pos > 0) {
$prev = $phpcsFile->findPrevious(
Expand All @@ -244,28 +268,98 @@ public function process(File $phpcsFile, $stackPtr)
&& $paramGroupid !== $groupid
) {
$error = 'Parameter tags must be grouped together in a doc comment';
$phpcsFile->addError($error, $tag, 'ParamGroup');
}
$fix = $phpcsFile->addFixableError($error, $tag, 'ParamGroup');

if ($fix === true) {
$phpcsFile->fixer->beginChangeset();

$thisTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $tag, $commentStart, false, $phpcsFile->eolChar));

if (isset($tokens[$stackPtr]['comment_tags'][($pos + 1)]) === true) {
$nextTag = $phpcsFile->findNext(T_DOC_COMMENT_TAG, ($tag + 1), $commentEnd);
$nextTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $nextTag, $commentStart, false, $phpcsFile->eolChar));
} else {
$nextTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $commentEnd, $commentStart, false, $phpcsFile->eolChar));
}

$thisTagContent = '';
for ($i = $thisTagLineStart; $i < $nextTagLineStart; $i++) {
if ($tokens[($i - 1)]['content'] === $phpcsFile->eolChar
&& $tokens[$i]['content'] === $indent
&& $tokens[($i + 1)]['content'] === '*'
&& $tokens[($i + 2)]['content'] === $phpcsFile->eolChar
) {
break;
}

$thisTagContent .= $tokens[$i]['content'];
$phpcsFile->fixer->replaceToken($i, '');
}

$insertionPointer = $phpcsFile->findPrevious(T_DOC_COMMENT_TAG, ($tag - 1), $commentStart, false, '@param');

while ($tokens[($insertionPointer - 1)]['content'] !== $phpcsFile->eolChar
|| $tokens[$insertionPointer]['content'] !== $indent
|| $tokens[($insertionPointer + 1)]['content'] !== '*'
|| $tokens[($insertionPointer + 2)]['content'] !== $phpcsFile->eolChar
) {
$insertionPointer++;

if (($insertionPointer + 2) >= $commentEnd) {
$insertionPointer = ($commentEnd - 1);
break;
}
}

$phpcsFile->fixer->addContentBefore($insertionPointer, $thisTagContent);

$phpcsFile->fixer->endChangeset();
}//end if
}//end if

if ($paramGroupid === null) {
$paramGroupid = $groupid;
}
} else if ($paramGroupid !== null && $firstTagAfterParams === null) {
$firstTagAfterParams = $tag;
}//end if

$tagGroups[$groupid][] = $tag;
}//end foreach

foreach ($tagGroups as $groupid => $group) {
$maxLength = 0;
$paddings = [];
$maxLength = 0;
$paddings = [];
$canFixNonParamGroup = true;
foreach ($group as $pos => $tag) {
if ($paramGroupid === $groupid
&& $tokens[$tag]['content'] !== '@param'
) {
$error = 'Tag %s cannot be grouped with parameter tags in a doc comment';
$data = [$tokens[$tag]['content']];
$phpcsFile->addError($error, $tag, 'NonParamGroup', $data);
}

if ($canFixNonParamGroup === true) {
$canFixNonParamGroup = $phpcsFile->addFixableError($error, $tag, 'NonParamGroup', $data);
} else {
$phpcsFile->addError($error, $tag, 'NonParamGroup', $data);
}

if ($canFixNonParamGroup === true) {
$phpcsFile->fixer->beginChangeset();

if ($pos > 0) {
$thisTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $tag, $commentStart, false, $phpcsFile->eolChar));
$phpcsFile->fixer->addContentBefore($thisTagLineStart, $indent.'*'.$phpcsFile->eolChar);
} else {
$nextTag = $phpcsFile->findNext(T_DOC_COMMENT_TAG, ($tag + 1), $commentEnd);
$nextTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $nextTag, $commentStart, false, $phpcsFile->eolChar));
$phpcsFile->fixer->addContentBefore($nextTagLineStart, $indent.'*'.$phpcsFile->eolChar);
}

$phpcsFile->fixer->endChangeset();
$canFixNonParamGroup = false;
}
}//end if

$tagLength = $tokens[$tag]['length'];
if ($tagLength > $maxLength) {
Expand All @@ -277,7 +371,7 @@ public function process(File $phpcsFile, $stackPtr)
if ($string !== false && $tokens[$string]['line'] === $tokens[$tag]['line']) {
$paddings[$tag] = $tokens[($tag + 1)]['length'];
}
}
}//end foreach

// Check that there was single blank line after the tag block
// but account for a multi-line tag comments.
Expand All @@ -298,7 +392,6 @@ public function process(File $phpcsFile, $stackPtr)
$phpcsFile->fixer->replaceToken($i, '');
}

$indent = str_repeat(' ', $tokens[$stackPtr]['column']);
$phpcsFile->fixer->addContent($prev, $phpcsFile->eolChar.$indent.'*'.$phpcsFile->eolChar);
$phpcsFile->fixer->endChangeset();
}
Expand Down Expand Up @@ -328,8 +421,45 @@ public function process(File $phpcsFile, $stackPtr)
// If there is a param group, it needs to be first.
if ($paramGroupid !== null && $paramGroupid !== 0) {
$error = 'Parameter tags must be defined first in a doc comment';
$phpcsFile->addError($error, $tagGroups[$paramGroupid][0], 'ParamNotFirst');
}
$fix = $phpcsFile->addFixableError($error, $tagGroups[$paramGroupid][0], 'ParamNotFirst');

if ($fix === true) {
$phpcsFile->fixer->beginChangeset();

$firstTag = $phpcsFile->findNext(T_DOC_COMMENT_TAG, $stackPtr, $commentEnd);
$firstParamTag = $phpcsFile->findNext(T_DOC_COMMENT_TAG, ($firstTag + 1), $commentEnd, false, '@param');
if ($firstTagAfterParams === null) {
$firstTagAfterParams = $commentEnd;
}

$lineBetween = ($tokens[$firstParamTag]['line'] - 1);

$tagGroupOne = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $firstTag, $commentStart, false, $phpcsFile->eolChar));
$tagGroupTwo = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $firstParamTag, $commentStart, false, $phpcsFile->eolChar));
$markerThree = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $firstTagAfterParams, $commentStart, false, $phpcsFile->eolChar));

$otherContent = $tokens[$tagGroupOne]['content'];
for ($i = ($tagGroupOne + 1); $i < $tagGroupTwo; $i++) {
if ($tokens[$i]['line'] === $lineBetween) {
break;
}

$otherContent .= $tokens[$i]['content'];
$phpcsFile->fixer->replaceToken($i, '');
}

$paramContent = $tokens[$tagGroupTwo]['content'];
for ($i = ($tagGroupTwo + 1); $i < $markerThree; $i++) {
$paramContent .= $tokens[$i]['content'];
$phpcsFile->fixer->replaceToken($i, '');
}

$phpcsFile->fixer->replaceToken($tagGroupOne, $paramContent);
$phpcsFile->fixer->replaceToken($tagGroupTwo, $otherContent);

$phpcsFile->fixer->endChangeset();
}//end if
}//end if

$foundTags = [];
foreach ($tokens[$stackPtr]['comment_tags'] as $pos => $tag) {
Expand All @@ -338,14 +468,46 @@ public function process(File $phpcsFile, $stackPtr)
$lastTag = $tokens[$stackPtr]['comment_tags'][($pos - 1)];
if ($tokens[$lastTag]['content'] !== $tagName) {
$error = 'Tags must be grouped together in a doc comment';
$phpcsFile->addError($error, $tag, 'TagsNotGrouped');
}
$fix = $phpcsFile->addFixableError($error, $tag, 'TagsNotGrouped');

if ($fix === true) {
$phpcsFile->fixer->beginChangeset();

$prevTag = $phpcsFile->findPrevious(T_DOC_COMMENT_TAG, ($tag - 1), $commentStart);
$prevTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $prevTag, $commentStart, false, $phpcsFile->eolChar));
$thisTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $tag, $commentStart, false, $phpcsFile->eolChar));

if (isset($tokens[$stackPtr]['comment_tags'][($pos + 1)]) === true) {
$nextTag = $phpcsFile->findNext(T_DOC_COMMENT_TAG, ($tag + 1), $commentEnd);
$nextTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $nextTag, $commentStart, false, $phpcsFile->eolChar));
} else {
$nextTagLineStart = (1 + $phpcsFile->findPrevious(T_DOC_COMMENT_WHITESPACE, $commentEnd, $commentStart, false, $phpcsFile->eolChar));
}

$prevTagContent = $tokens[$prevTagLineStart]['content'];
for ($i = ($prevTagLineStart + 1); $i < $thisTagLineStart; $i++) {
$prevTagContent .= $tokens[$i]['content'];
$phpcsFile->fixer->replaceToken($i, '');
}

$thisTagContent = $tokens[$thisTagLineStart]['content'];
for ($i = ($thisTagLineStart + 1); $i < $nextTagLineStart; $i++) {
$thisTagContent .= $tokens[$i]['content'];
$phpcsFile->fixer->replaceToken($i, '');
}

$phpcsFile->fixer->replaceToken($prevTagLineStart, $thisTagContent);
$phpcsFile->fixer->replaceToken($thisTagLineStart, $prevTagContent);

$phpcsFile->fixer->endChangeset();
}//end if
}//end if

continue;
}
}//end if

$foundTags[$tagName] = true;
}
}//end foreach

}//end process()

Expand Down
29 changes: 27 additions & 2 deletions src/Standards/Generic/Tests/Commenting/DocCommentUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@
* Short description.
*
*
* @param
* @param one
*
* @param
* @param two
*
*
* @tag one
Expand Down Expand Up @@ -163,6 +163,27 @@
* @three bar
*/

/**
* @one
* @two
* @three
* @four
* @one
* @two
* @three
* @four
*/

/**
* @one this has more than one line (1)
* and should remain with the first tag (1)
* @two a different tag which should be (2)
* moved to after the third tag (2)
* @one the same tag as the first but listed (3)
* after a different tag; this should be (3)
* moved above the second tag (3)
*/

/**
* @ var Comment
*/
Expand Down Expand Up @@ -249,4 +270,8 @@
* @link http://pear.php.net/package/PHP_CodeSniffer
*/

/**
*
*/

/** No docblock close tag. Must be last test without new line.
Loading