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

Generic.Commenting.Todo.CommentFound sample not working #3769

Open davidsneighbour opened 1 year ago

davidsneighbour commented 1 year ago

Describe the bug

I configured my Generic.Commenting.Todo.CommentFound setup according to the docs in the wiki..

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="Booka" namespace="Booka">
  <!-- https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset -->
  <file>./src/</file>
  <exclude-pattern type="relative">^/src/Frontend/Forms/*</exclude-pattern>

  <arg name="basepath" value="." />
  <arg name="colors" />
  <arg name="parallel" value="75" />
  <arg name="severity" value="1" />
  <arg name="report" value="full" />
  <arg value="sp" />

  <rule ref="Generic.Commenting.Todo.CommentFound">
    <message>Please review this TODO comment: %s</message>
    <severity>3</severity>
  </rule>

</ruleset>

There is more in the config, but I left it out for readability (full config at the end).

I would expect the following to throw an error like Please review this TODO comment: @todo find out if this maybe is better suited at another location or Please review this TODO comment: find out if this maybe is better suited at another location. But it throws only Please review this TODO comment:. It's notable in VSCode and on the CLI that two spaces are there at the end, as if %s is returning empty at that point.

Code sample

/**
 * @todo find out if this maybe is better suited at another location
 */
somefunction();

This happens with ANY @todo comment comment.

Versions (please complete the following information):

Complete config:

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="Booka" namespace="Booka">
  <!-- https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset -->
  <file>./src/</file>
  <exclude-pattern type="relative">^/src/Frontend/Forms/*</exclude-pattern>

  <arg name="basepath" value="." />
  <arg name="colors" />
  <arg name="parallel" value="75" />
  <arg name="severity" value="1" />
  <arg name="report" value="full" />
  <arg value="sp" />

  <rule ref="Generic.WhiteSpace.ScopeIndent">
    <properties>
      <property name="indent" value="2" />
      <property name="tabIndent" value="false" />
    </properties>
  </rule>
  <rule ref="Generic.WhiteSpace.DisallowTabIndent" />

  <rule ref="Generic.Commenting.Todo.CommentFound">
    <message>Please review this TODO comment: %s</message>
    <severity>3</severity>
  </rule>

  <rule ref="PSR2.Methods.FunctionCallSignature">
    <properties>
      <property name="indent" value="2" />
      <property name="allowMultipleArguments" value="false" />
    </properties>
  </rule>

  <rule ref="PSR12" />
  <rule ref="Modernize" />
  <rule ref="NormalizedArrays" />
  <rule ref="Universal.Arrays.DuplicateArrayKey" />
  <rule ref="Universal.Arrays.MixedArrayKeyTypes" />
  <rule ref="Universal.Arrays.MixedKeyedUnkeyedArray" />
  <rule ref="Universal.Classes.DisallowAnonClassParentheses" />

</ruleset>
jrfnl commented 1 year ago

@davidsneighbour I've just tested and confirmed this issue, however, if you look at the sniff, you will see that the %s is only added when the sniff could figure out a "todo message" and if the Generic.Commenting.Todo sniff is run over the same code sample without a custom message, the "todo message" also doesn't show.

To me, this looks more like something which needs to be improved in the sniff itself.

Most notably - it looks like the sniff looks at all comments and checks if they start with @todo and not specifically at comments marked with a @todo tag, which is tokenized differently.

jrfnl commented 1 year ago

@davidsneighbour I've opened PR #3771 with some improvements for the Generic.Commenting.Todo sniff. I believe this should largely fix your concern. Testing appreciated.

Note: when there is a "todo description", the error code for the sniff is different (and always was), so your ruleset would not work as-is, you'll need to update it to:

  <!-- This error code is for when no description is found. -->
  <rule ref="Generic.Commenting.Todo.CommentFound">
    <message>Please review this TODO comment</message>
    <severity>3</severity>
  </rule>
  <!-- This error code is used when a task description is found. -->
  <rule ref="Generic.Commenting.Todo.TaskFound">
    <message>Please review this TODO comment: %s</message>
    <severity>3</severity>
  </rule>
stevenfoncken commented 11 months ago

@jrfnl

The ruleset.xml example on the "Annotated-Ruleset" wiki page should also be edited. (https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset#the-annotated-sample-file)

Line 215-218

<rule ref="Generic.Commenting.Todo.CommentFound">
 <message>Please review this TODO comment: %s</message>
 <severity>3</severity>
</rule>

Should be:

<rule ref="Generic.Commenting.Todo.TaskFound">
 <message>Please review this TODO comment: %s</message>
 <severity>3</severity>
</rule>

And CommentFound in the comment above the rule snippet should be TaskFound.

jrfnl commented 10 months ago

@stevenfoncken Thanks for pointing that out. I've updated the wiki page now.