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

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

Open
davidsneighbour opened this issue Feb 25, 2023 · 4 comments · May be fixed by PHPCSStandards/PHP_CodeSniffer#53
Open

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

davidsneighbour opened this issue Feb 25, 2023 · 4 comments · May be fixed by PHPCSStandards/PHP_CodeSniffer#53
Labels
Standard: Generic

Comments

@davidsneighbour
Copy link

davidsneighbour commented Feb 25, 2023 •
edited
Loading

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):

  • OS: Ubuntu 22.10
  • PHP: 8.1
  • PHPCS: PHP_CodeSniffer version 3.7.2 (stable) by Squiz (http://www.squiz.net)
  • Installed via composer
  • Standard: PSR12

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
Copy link
Contributor

jrfnl commented Mar 2, 2023

@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 jrfnl mentioned this issue Mar 2, 2023
Closed
@jrfnl
Copy link
Contributor

jrfnl commented Mar 2, 2023

@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
Copy link

stevenfoncken commented Oct 14, 2023 •
edited
Loading

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

jrfnl commented Oct 28, 2023

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

@jrfnl jrfnl linked a pull request Nov 9, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Standard: Generic
Projects
None yet
Milestone
No milestone
3 participants
@davidsneighbour @jrfnl @stevenfoncken