Create @todo format sniff.

[adamzimmermann]

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

[jonathan1055]

Nice work. Will review shortly. I know that Klausi will want test coverage for this too.

[jonathan1055]

Did you actually run your latest changes? I get nothing reported at all, for both types of bad format. I use

phpcs --standard=Drupal -vs --colors --sniffs=Drupal.Commenting.TodoComment path/to/testfile.php

[jonathan1055]

* Will the `// ` be in the string that is sent to the sniff? If so, I need to adjust it.

Yes // will be in the content for T_COMMENT, but in the not in the string returned by findNext(T_DOC_COMMENT_STRING

* Should we enforce a capital letter after `@todo`? (I went ahead and did this.)

I'd say yes, and you also enforced one space. These are all good, but I don't know if they are actually written in the standard?

[jonathan1055]

The above will give the pointer not the comment text. You need something like

$pointer = $phpcsFile->findNext(T_DOC_COMMENT_STRING, ($stackPtr + 1));
$comment = $tokens[$pointer]['content'];

[adamzimmermann]

@jonathan1055 thanks for all of the great feedback. 🙏

Did you actually run your latest changes?

I ran them a few regex iterations ago. I wanted to get a review on the high level approach first. Clearly I need to do some actual testing again though.

I'll work on some fixes in the next couple days and report back.

[jonathan1055]

Nice work. I started looking at your changes, but now there are lots, I think I will wait until you say you are ready for review. Let me know.

[adamzimmermann]

I was hoping this last commit would fix all of the remaining Travis issue, but it failed to build. I don't see a re-run tests option. So I might need some assistance here.

Either way, I believe this is ready for review.

The only other thing I can think to add is the ability to fix the issue automatically, but that might be a follow up PR.

[jonathan1055]

I was hoping this last commit would fix all of the remaining Travis issue, but it failed to build. I don't see a re-run tests option. So I might need some assistance here.

I created a local branch with all your changes and tried vendor/phpunit/phpunit/phpunit -v --filter=todo
Yes there was a big problem, nothing happened! apart from the fan on my laptop starting to whirr. Ctnl C.

So I looked at the test file, and noticed in TodoCommentUnitTest.php getErrorList() your definition of $errorList had
for ($i = 25; $i < 100; $i + 4) { and the problem was this loop never finished. You need $i += 4
Then the tests at least ran but gave many failures.

Your recent change to split all the incorrect docbock @todo into a separate comment block (not sure why you did that) but anyway, these are not being found as valid docblocks because they only have /* as the first row, they all need /**. With these two changes most of the tests passed. I suggest we make just these two fixes, and push a commit here so we can see the test run. Then we can work on the remainder of the review and the test failures. I may be able to push to this PR, I don't know.

The only other thing I can think to add is the ability to fix the issue automatically, but that might be a follow up PR.

Definitely leave that until the main work is done.

[adamzimmermann]

I created a local branch with all your changes and tried vendor/phpunit/phpunit/phpunit -v --filter=todo Yes there was a big problem, nothing happened! apart from the fan on my laptop starting to whirr. Ctnl C.

That's not what I wanted haha. I was running the sniffs locally, but wasn't running the tests locally and it shows haha. I'll get the tests running locally too as your next comment indicates there are still issues beyond the big one.

So I looked at the test file, and noticed in TodoCommentUnitTest.php getErrorList() your definition of $errorList had
for ($i = 25; $i < 100; $i + 4) { and the problem was this loop never finished. You need $i += 4
Then the tests at least ran but gave many failures.

🤦 That would do it.

Your recent change to split all the incorrect docbock @todo into a separate comment block (not sure why you did that) but anyway, these are not being found as valid docblocks because they only have /* as the first row, they all need /**. With these two changes most of the tests passed. I suggest we make just these two fixes, and push a commit here so we can see the test run. Then we can work on the remainder of the review and the test failures. I may be able to push to this PR, I don't know.

I had to do that to make the code alignment sniff happy. Some of the malformed statements were causing it to bark about how other things were indented, so I separated them. I was struggling with the formatting as the standards enforced in this repo are very different from the code style I'm used to working with.

I'll try making the comments use /**, but I seem to remember it barking about some other formatting issues then.

I was seeing Inline doc block comments are not allowed; use "/* Comment */" or "// Comment" instead. I added a bunch of functions to get around this.

Definitely leave that until the main work is done.

👍

I'll get some updates pushed in the next couple days (maybe later today 🤞).

[adamzimmermann]

Everything seems to be passing. ✅

[jonathan1055]

I had to do that to make the code alignment sniff happy. Some of the malformed statements were causing it to bark about how other things were indented, so I separated them. I was struggling with the formatting as the standards enforced in this repo are very different from the code style I'm used to working with.

Yes I saw in your commits you were having to work at it. However, the test files are not actually part of the standard phpcs scan so they are not subject to this checking. But unfortunately you had named yours TodoCommentUnitTest.inc.php so it was being included, and hence caused you trouble. The test files should be named .inc without the .php, then coder's phpcs.xml.dist file with ignore them due to

    <!-- Test files that should not be checked. -->
    <exclude-pattern>tests/*\.(inc|css|js|api\.php|tpl\.php)$</exclude-pattern>

So, you can return to a simple test files, and I've done this in my copy of your branch, to save you the trouble - see
https://github.com/jonathan1055/coder/tree/118-todo-17
You should be able to merge those changes into yours using https://github.com/adamzimmermann/coder/compare/8.x-3.x...jonathan1055:118-todo-17?expand=1 or I can provide a patch to take you from your current state (after 19 commits) up to this latest version if you want it. I slightly modified a couple of the valid test cases, to show that $ and \ are accepted as first char of the todo text. I also added the standard method of producing debug, so we can run on a small test file and get detailed output locally of what is happening.

I have a couple more questions but lets get the simple test file merged into your branch first.

[jonathan1055]

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

1. I have reformatted the regex to allow freespace and commenting. This is much easier to understand and alter
2. 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
3. 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
4. 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.
5. Just for the record @to-do, @to - do, @to- --- do are all considered to be @todo tags and are therefore checked (and failed)
6. @to and @to- are not considered to be @todo tags, and are not checked, which is correct.
7. 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.

[adamzimmermann]

Hoping to find time to get back to this today. Thanks for the contributions!

[jonathan1055]

OK. If you want to discuss anything, I have just started using Slack. The coder channel is https://app.slack.com/client/T06GX3JTS/CJ19MV96J

[adamzimmermann]

👍 What is the subdomain?

[jonathan1055]

drupal.slack.com
I can see your id, and just added you to #coder channel

[adamzimmermann]

Closing in favor of #120.

[jonathan1055]

You made the comment:
"I was struggling with the formatting as the standards enforced in this repo are very different from the code style I'm used to working with."

I forgot to say at the time that, yes, this standard is not what I'm used to. However, running phpcbf can automatically fix a large proportion of the formatting requirements that we often miss, and really eases the hassle out of it.