search

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

Handle `@param` in docblock for variables passed by reference #3813

Closed fredden closed 1 year ago

fredden commented 1 year ago

This started as an auto-fix to remove ampersands from @param tags in docblocks within Squiz.Commenting.FunctionComment. This has evolved into "handle @param tags for variables passed by reference." We are also increasing test coverage of related-but-not-changed behaviour.

fredden commented 1 year ago

PSR-19 makes no mention of variables passed by reference. Based on this, I'll update this pull request to instead ignore this character and not auto-fix.

fredden commented 1 year ago

MissingParamTag - I have adjusted the sniff to not throw this when the variable is prefixed incorrectly in the @param tag. Should this still be thrown when only the case mismatches? For example,

1) PHP_CodeSniffer\Standards\Squiz\Tests\Commenting\FunctionCommentUnitTest::testSniff
[LINE 1050] Expected 0 error(s) in FunctionCommentUnitTest.inc but found 1 error(s). The error(s) found were:
 -> Doc comment for parameter "$CASE" missing (Squiz.Commenting.FunctionComment.MissingParamTag)
[LINE 1063] Expected 0 error(s) in FunctionCommentUnitTest.inc but found 1 error(s). The error(s) found were:
 -> Doc comment for parameter &$case does not match case of actual variable name &$CASE (Squiz.Commenting.FunctionComment.ParamNameNoCaseMatch)
jrfnl commented 1 year ago

MissingParamTag - I have adjusted the sniff to not throw this when the variable is prefixed incorrectly in the @param tag. Should this still be thrown when only the case mismatches? For example,

Parameters in PHP are case-sensitive, so yes, case mismatches should IMO definitely be flagged.

fredden commented 1 year ago

Thanks for your time & patience on this one @jrfnl. I appreciate the feedback etc.

jrfnl commented 1 year ago

@fredden Would you mind rebasing this PR (and possibly squashing the commits) to get it past the merge conflict ?

fredden commented 1 year ago

@fredden Would you mind rebasing this PR (and possibly squashing the commits) to get it past the merge conflict ?

Yes, no problem. I'll look at this today.

jrfnl commented 9 months ago

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).