From 0d6f3ed58dc79c6e0d644b5688e60c4a34f04346 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 2 Mar 2023 22:09:40 +0100 Subject: [PATCH 1/3] Generic/Todo-Fixme: make the sniffs more selective The sniffs as-they-were, would sniff all comment related tokens, including `T_DOC_COMMENT_STAR`, `T_DOC_COMMENT_WHITESPACE` etc. Sniffing those tokens is redundant and makes the sniffs slower than is needed. Fixed now by making the tokens being registered by the sniffs more selective/targetted. --- src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php | 7 ++++++- src/Standards/Generic/Sniffs/Commenting/TodoSniff.php | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php b/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php index b78a6595ee..c0726833a1 100644 --- a/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php +++ b/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php @@ -35,7 +35,12 @@ class FixmeSniff implements Sniff */ public function register() { - return array_diff(Tokens::$commentTokens, Tokens::$phpcsCommentTokens); + return [ + T_COMMENT, + T_DOC_COMMENT, + T_DOC_COMMENT_TAG, + T_DOC_COMMENT_STRING, + ]; }//end register() diff --git a/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php b/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php index d24111cbd3..c0e463a4f0 100644 --- a/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php +++ b/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php @@ -34,7 +34,12 @@ class TodoSniff implements Sniff */ public function register() { - return array_diff(Tokens::$commentTokens, Tokens::$phpcsCommentTokens); + return [ + T_COMMENT, + T_DOC_COMMENT, + T_DOC_COMMENT_TAG, + T_DOC_COMMENT_STRING, + ]; }//end register() From 302bd283ed0f5acf5545ce4e711222e4cdf7a2c4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 2 Mar 2023 22:42:40 +0100 Subject: [PATCH 2/3] Generic/Todo: improve handling of "todo" tags in docblocks Until now, the sniff would only examine an individual comment token, while when a `@todo` tag is used in a docblock, the "task description" is normally in the next `T_DOC_COMMENT_STRING` token. This commit fixes this and the sniff will now take docblock `@todo` tags into account. Includes additional unit tests. --- .../Generic/Sniffs/Commenting/TodoSniff.php | 48 ++++++++++++------- .../Generic/Tests/Commenting/TodoUnitTest.inc | 12 +++++ .../Generic/Tests/Commenting/TodoUnitTest.js | 12 +++++ .../Generic/Tests/Commenting/TodoUnitTest.php | 5 ++ 4 files changed, 61 insertions(+), 16 deletions(-) diff --git a/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php b/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php index c0e463a4f0..e5d7db9d79 100644 --- a/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php +++ b/src/Standards/Generic/Sniffs/Commenting/TodoSniff.php @@ -11,7 +11,6 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; -use PHP_CodeSniffer\Util\Tokens; class TodoSniff implements Sniff { @@ -55,27 +54,44 @@ public function register() */ public function process(File $phpcsFile, $stackPtr) { - $tokens = $phpcsFile->getTokens(); - + $tokens = $phpcsFile->getTokens(); $content = $tokens[$stackPtr]['content']; $matches = []; - preg_match('/(?:\A|[^\p{L}]+)todo([^\p{L}]+(.*)|\Z)/ui', $content, $matches); - if (empty($matches) === false) { - // Clear whitespace and some common characters not required at - // the end of a to-do message to make the warning more informative. - $type = 'CommentFound'; - $todoMessage = trim($matches[1]); - $todoMessage = trim($todoMessage, '-:[](). '); - $error = 'Comment refers to a TODO task'; - $data = [$todoMessage]; - if ($todoMessage !== '') { - $type = 'TaskFound'; - $error .= ' "%s"'; + + if (preg_match('/(?:\A|[^\p{L}]+)todo([^\p{L}]+(.*)|\Z)/ui', $content, $matches) !== 1) { + return; + } + + // Clear whitespace and some common characters not required at + // the end of a to-do message to make the warning more informative. + $todoMessage = trim($matches[1]); + $todoMessage = trim($todoMessage, '-:[](). '); + + if ($tokens[$stackPtr]['code'] === T_DOC_COMMENT_TAG + && $todoMessage === '' + ) { + $nextNonEmpty = $phpcsFile->findNext(T_DOC_COMMENT_WHITESPACE, ($stackPtr + 1), null, true); + if ($nextNonEmpty !== false + && $tokens[$nextNonEmpty]['code'] === T_DOC_COMMENT_STRING + ) { + $todoMessage = trim($tokens[$nextNonEmpty]['content'], '-:[](). '); } + } + + $error = 'Comment refers to a TODO task'; + $type = 'CommentFound'; + $data = [$todoMessage]; + if ($todoMessage !== '') { + $error .= ' "%s"'; + $type = 'TaskFound'; + } - $phpcsFile->addWarning($error, $stackPtr, $type, $data); + if ($tokens[$stackPtr]['code'] === T_DOC_COMMENT_TAG) { + $type .= 'InDocblock'; } + $phpcsFile->addWarning($error, $stackPtr, $type, $data); + }//end process() diff --git a/src/Standards/Generic/Tests/Commenting/TodoUnitTest.inc b/src/Standards/Generic/Tests/Commenting/TodoUnitTest.inc index 64c624cead..d2a69f0b15 100644 --- a/src/Standards/Generic/Tests/Commenting/TodoUnitTest.inc +++ b/src/Standards/Generic/Tests/Commenting/TodoUnitTest.inc @@ -21,3 +21,15 @@ Debug::bam('test'); //TODO. //étodo //todoé + +/** + * @todo This message should be picked up. + * @todo: This message should be picked up too. + * @todo - here is a message + * + * The below should not show a message as there is no description associated with the tag. + * @todo + * @anothertag + * + * @param string $something TODO: add description + */ diff --git a/src/Standards/Generic/Tests/Commenting/TodoUnitTest.js b/src/Standards/Generic/Tests/Commenting/TodoUnitTest.js index 8f01ca17f3..0133233c57 100644 --- a/src/Standards/Generic/Tests/Commenting/TodoUnitTest.js +++ b/src/Standards/Generic/Tests/Commenting/TodoUnitTest.js @@ -21,3 +21,15 @@ alert('test'); //TODO. //étodo //todoé + +/** + * @todo This message should be picked up. + * @todo: This message should be picked up too. + * @todo - here is a message + * + * The below should not show a message as there is no description associated with the tag. + * @todo + * @anothertag + * + * @param string $something TODO: add description + */ diff --git a/src/Standards/Generic/Tests/Commenting/TodoUnitTest.php b/src/Standards/Generic/Tests/Commenting/TodoUnitTest.php index d3300fb15c..bdbf785c70 100644 --- a/src/Standards/Generic/Tests/Commenting/TodoUnitTest.php +++ b/src/Standards/Generic/Tests/Commenting/TodoUnitTest.php @@ -54,6 +54,11 @@ public function getWarningList() 16 => 1, 18 => 1, 21 => 1, + 26 => 1, + 27 => 1, + 28 => 1, + 31 => 1, + 34 => 1, ]; }//end getWarningList() From 0f89212145cf4aff499b8b68299e7fd331902c99 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 2 Mar 2023 22:43:41 +0100 Subject: [PATCH 3/3] Generic/Fixme: improve handling of "fixme" tags in docblocks Essentially the same fix as applied in the sister-commit for the `Generic.Commenting.Todo` sniff. Until now, the sniff would only examine an individual comment token, while when a `@fixme` tag is used in a docblock, the "task description" is normally in the next `T_DOC_COMMENT_STRING` token. This commit fixes this and the sniff will now take docblock `@fixme` tags into account. Includes additional unit tests. --- .../Generic/Sniffs/Commenting/FixmeSniff.php | 48 ++++++++++++------- .../Tests/Commenting/FixmeUnitTest.inc | 14 ++++++ .../Generic/Tests/Commenting/FixmeUnitTest.js | 14 ++++++ .../Tests/Commenting/FixmeUnitTest.php | 5 ++ 4 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php b/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php index c0726833a1..9398742899 100644 --- a/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php +++ b/src/Standards/Generic/Sniffs/Commenting/FixmeSniff.php @@ -12,7 +12,6 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; -use PHP_CodeSniffer\Util\Tokens; class FixmeSniff implements Sniff { @@ -56,27 +55,44 @@ public function register() */ public function process(File $phpcsFile, $stackPtr) { - $tokens = $phpcsFile->getTokens(); - + $tokens = $phpcsFile->getTokens(); $content = $tokens[$stackPtr]['content']; $matches = []; - preg_match('/(?:\A|[^\p{L}]+)fixme([^\p{L}]+(.*)|\Z)/ui', $content, $matches); - if (empty($matches) === false) { - // Clear whitespace and some common characters not required at - // the end of a fixme message to make the error more informative. - $type = 'CommentFound'; - $fixmeMessage = trim($matches[1]); - $fixmeMessage = trim($fixmeMessage, '-:[](). '); - $error = 'Comment refers to a FIXME task'; - $data = [$fixmeMessage]; - if ($fixmeMessage !== '') { - $type = 'TaskFound'; - $error .= ' "%s"'; + + if (preg_match('/(?:\A|[^\p{L}]+)fixme([^\p{L}]+(.*)|\Z)/ui', $content, $matches) !== 1) { + return; + } + + // Clear whitespace and some common characters not required at + // the end of a to-do message to make the warning more informative. + $fixmeMessage = trim($matches[1]); + $fixmeMessage = trim($fixmeMessage, '-:[](). '); + + if ($tokens[$stackPtr]['code'] === T_DOC_COMMENT_TAG + && $fixmeMessage === '' + ) { + $nextNonEmpty = $phpcsFile->findNext(T_DOC_COMMENT_WHITESPACE, ($stackPtr + 1), null, true); + if ($nextNonEmpty !== false + && $tokens[$nextNonEmpty]['code'] === T_DOC_COMMENT_STRING + ) { + $fixmeMessage = trim($tokens[$nextNonEmpty]['content'], '-:[](). '); } + } + + $error = 'Comment refers to a FIXME task'; + $type = 'CommentFound'; + $data = [$fixmeMessage]; + if ($fixmeMessage !== '') { + $error .= ' "%s"'; + $type = 'TaskFound'; + } - $phpcsFile->addError($error, $stackPtr, $type, $data); + if ($tokens[$stackPtr]['code'] === T_DOC_COMMENT_TAG) { + $type .= 'InDocblock'; } + $phpcsFile->addError($error, $stackPtr, $type, $data); + }//end process() diff --git a/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.inc b/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.inc index 69ab337810..0a2ee05bc1 100644 --- a/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.inc +++ b/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.inc @@ -21,3 +21,17 @@ Debug::bam('test'); //FIXME. //éfixme //fixmeé + +/** + * While there is no official "fix me" tag, only `@todo`, let's support a tag for the purpose of this sniff anyway. + * + * @fixme This message should be picked up. + * @fixme: This message should be picked up too. + * @fixme - here is a message + * + * The below should not show a message as there is no description associated with the tag. + * @fixme + * @anothertag + * + * @param string $something FIXME: add description + */ diff --git a/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.js b/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.js index 7836b9a213..f086873105 100644 --- a/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.js +++ b/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.js @@ -21,3 +21,17 @@ alert('test'); //FIXME. //éfixme //fixmeé + +/** + * While there is no official "fix me" tag, only `@todo`, let's support a tag for the purpose of this sniff anyway. + * + * @fixme This message should be picked up. + * @fixme: This message should be picked up too. + * @fixme - here is a message + * + * The below should not show a message as there is no description associated with the tag. + * @fixme + * @anothertag + * + * @param string $something FIXME: add description + */ diff --git a/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.php b/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.php index 90ee54c499..70e98e1364 100644 --- a/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.php +++ b/src/Standards/Generic/Tests/Commenting/FixmeUnitTest.php @@ -40,6 +40,11 @@ public function getErrorList() 16 => 1, 18 => 1, 21 => 1, + 28 => 1, + 29 => 1, + 30 => 1, + 33 => 1, + 36 => 1, ]; }//end getErrorList()