Create @todo format sniff. #120

adamzimmermann · 2020-10-07T19:32:03Z

Adds @todo format sniff.

Drupal.org issue:

https://www.drupal.org/project/coder/issues/2222435

Testing/Examples:

https://regex101.com/r/cir1XP/9

Questions:
~~Will the // be in the string that is sent to the sniff? If so, I need to adjust it.~~
~~- Should we enforce a capital letter after @todo? (I went ahead and did this.)~~

Closing #118 in favor of this PR to simplify/avoid merge conflicts from this PR where the following was done:

I have made a few more suggestions and pushed them to my branch.

I have reformatted the regex to allow freespace and commenting. This is much easier to understand and alter
T_COMMENT matches ordinary comments in // and this includes the // so we do need to cater for // in the regex. Howvwre I have allowed for any number in case we get /// etc
T_DOC_COMMENT_STRING find all comments lines in a docblock. This does not inlcude the starting * so there no need to cater for that in the regex
T_DOC_COMMENT_TAG finds all doc block tags, i.e. @anything within a /** doc block. You correctly build up the the rest of the line of text in a loop, to ensure we get the spacing accurate. Unfortunately this text will also be found later in the procesing because it matches T_DOC_COMMENT_STRING but it will not be reported twice as the @todo wont be found. It's just a minor ineficiency that it gets tested twice.
Just for the record @to-do, @to - do, @to- --- do are all considered to be @todo tags and are therefore checked (and failed)
@to and @to- are not considered to be @todo tags, and are not checked, which is correct.
If there is no @ in front of the todo then I think we should only count todo and to-do as valid @todo tags to be tested. Otherwise we would match and report on the second line of the following comment which is clearly not a todo tag:
// This is not a todo tag. It is a general comment and we do not want
// to do the standards checking here.
I have made this change in the regex, and included test coverage.
8. Given that this @todo format is only advisory, should these all be coding standards warnings not errors?

Overall excelent work, this will be a good addition. I hope you can easily compare my branch to yours and get the changes? If you want patches let me know.

… dup test cases

Amend comments in regex, add test for normal to do in comment, remove…

tests/Drupal/Commenting/TodoCommentUnitTest.php

jonathan1055 · 2020-10-07T20:35:48Z

Tests pass, that's great.

adamzimmermann · 2020-10-07T20:40:52Z

I believe this is ready for final review and merging. @jonathan1055 thanks for all of your feedback and help on this.

jonathan1055 · 2020-10-08T07:08:48Z

Yes this all looks good. I think you need to add a new comment on the drupal issue (a) to link to this new PR and (b) to highlight that it is ready for @arkener or @klausi to review..

klausi

Nice, thanks a lot!

I will do a test run against Drupal core to check if I see any false positives.

klausi · 2020-10-17T11:33:53Z

coder_sniffer/Drupal/Sniffs/Commenting/TodoCommentSniff.php

+    {
+        $debug = Config::getConfigData('todo_debug');
+        if ($debug !== null) {
+            $this->debug = (bool) $debug;


I think we should not initialize this variable on every single process() call? Can we do that once in the constructor instead?

Ah, I see that this approach is copied from ScopeIndentSniff, so also not ideal there. Anyway, then I think it is fine to keep this as is.

Yes and I also used it in Commenting/DeprecatedSniff Would you like me to start a new issus to change all three of these, when this is committed?

klausi · 2020-10-17T11:38:16Z

coder_sniffer/Drupal/Sniffs/Commenting/TodoCommentSniff.php

+            }
+
+            $comment = trim($comment, " /\r\n");
+            $phpcsFile->addWarning("'%s' should match the format '@todo Some task'", $stackPtr, 'TodoFormat', [$comment]);


Comments should end with a dot, also todo comments. So the example should be something like @todo Fix problem X here.

It would be really cool to have a fixer here, that would help many people auto-fix this standard (also in Drupal core). I think it should be possible, we should have all info in the regex and could try a preg_replace().

Not a blocker for this pull request, we can merge this as first step.

Yes, doc comments not ending with a period is checked for. But currently @ tags are not considered within that group, so no warning is given if they have no period at the end. I agree that if they should end in a dot then this example should include the dot. But then we should also include @ tags in the general sniff for no ending period. @see should not be included, we discussed that on the deprecation sniffs, the url should not end in a dot. Maybe this is for another issue?

Regarding a fixer, yes it should be possible. @ adamzimmermann did suggest that, but we wanted to wait until we knew you were interested in this sniff

I see, very considerate of you! Yep, I absolutely support this sniff.

Also yep to the ending dot discussion, should be a separate issue.

Also yep to the ending dot discussion, should be a separate issue.

I have raised https://www.drupal.org/project/coder/issues/3183156 to check for doc tags not ending with a period.

klausi · 2020-10-17T12:03:10Z

All warnings found in Drupal core look correct to me, yay! Will merge this now and improve the warning message as I proposed.

jonathan1055 and others added 6 commits September 28, 2020 14:31

after 19 commits

5afcab7

Simplify test file

9abf898

Format regex as multi-line with comments.

e7934bd

fix coding standards in sniff

4f82878

Merge pull request #1 from jonathan1055/118-todo-17

7f53f79

Amend comments in regex, add test for normal to do in comment, remove…

6b2dff4

… dup test cases

adamzimmermann mentioned this pull request Oct 7, 2020

Create @todo format sniff. #118

Closed

adamzimmermann added 2 commits October 7, 2020 15:08

Merge pull request #2 from jonathan1055/118-todo-17

eba1761

Amend comments in regex, add test for normal to do in comment, remove…

Change error to warning.

cdcf84d

adamzimmermann marked this pull request as ready for review October 7, 2020 20:20

Fix typo.

1f56ef0

jonathan1055 reviewed Oct 7, 2020

View reviewed changes

tests/Drupal/Commenting/TodoCommentUnitTest.php Outdated Show resolved Hide resolved

Change variable name.

9db45e1

klausi reviewed Oct 17, 2020

View reviewed changes

klausi merged commit 1e93d2e into pfrenssen:8.x-3.x Oct 17, 2020

Create @todo format sniff. #120

Create @todo format sniff. #120

Uh oh!

Conversation

adamzimmermann commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jonathan1055 commented Oct 7, 2020

Uh oh!

adamzimmermann commented Oct 7, 2020

Uh oh!

jonathan1055 commented Oct 8, 2020

Uh oh!

klausi left a comment

Choose a reason for hiding this comment

Uh oh!

klausi Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

jonathan1055 Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

klausi Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

klausi Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

jonathan1055 Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

klausi Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

jonathan1055 Nov 18, 2020

Choose a reason for hiding this comment

Uh oh!

klausi commented Oct 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamzimmermann commented Oct 7, 2020 •

edited

Loading