Add a sniff to ensure @todo comments follow the coding standards

Big +1 for this. I just want to say that this is the most common thing I see missed after a new developer is setup with the sniffs in PhpStorm. Most everyone does it differently and their code always fails code review on this @todo, @TODO, @TODO:, @todo:.

Add summary link to @todo standard documentation, here => https://www.drupal.org/node/1354#todo.

+1 from me..
As someone who often writes POC code with lots of details to be fleshed out at a later date, I think this is valuable.

Something I just noticed. The following code returns an error, even though it is following the coding standards:

    // @todo Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
    //   nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed
    //   diam voluptua.

26 | ERROR | Comment indentation error, expected only 1 spaces

The code below passes when it shouldn't:

    // @todo Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam
    // nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed
    // diam voluptua.

Coding standards indeed suggest that it should be valid when comments of a multi-line @todo, for the second line and more, start with two extra space characters.

As a workaround to ignore these warning, I simply surround multi-line @todo comments with // phpcs:disable and // phpcs:enable.

I haven't used this extensively yet, but this sniff seems to work well in my testing so far.

Regex Testing:
https://regex101.com/r/cir1XP/2

Screenshot:
https://user-images.githubusercontent.com/1349906/88872530-fcf44300-d1df...

This doesn't test multi-line comments. It only addresses the format of the @todo text.

@adamzimmermann This looks great. I have tested the patch on real code, and in principle it works. Just noted a few things:

1. The regex testing link has 'g' global flag, but your patch does not. Is that intended?
2. In the issue summary we have examples of : and - after the 'todo' before the text. It would be nice to report these as problems too.
3. Also should we treat 'todo' without a leading @ as an intended todo and include it in the checking? That could open up too many unwanted line matches, so not sure about that.
4. @todo in a doc block are not covered yet. This only check code standalone comments. It would be very good to have docblock @todo covered as they are potentially just as common as standalone comments

But overall a great start. I hesitate to change this to 'needs work' as there is nothing actually wrong with the work so far, it just needs to be expanded, I think.

[nice regex test site, I had not seen that before. Very useful]

I have updated the regex with some suggestions - see https://regex101.com/r/cir1XP/3

• The central \s will report @to do
• ? will allow any multiples, so @to- do is also reported
• Added \s\w after the final @todo so that only spaces are allowed before the text, i.e. no : or -
• The above \s\w has the nice side-effect that an accidental empty @todo with nothing after it also fails the sniff

Hope it's OK that I updated your regex links. Thanks.

The regex testing link has 'g' global flag, but your patch does not. Is that intended?

I assumed I didn't need it since it was being fed things line by line, or that was my assumption. Perhaps that is incorrect?

In the issue summary we have examples of : and - after the 'todo' before the text. It would be nice to report these as problems too.

Good call. Sounds like you just fixed this!

Also should we treat 'todo' without a leading @ as an intended todo and include it in the checking? That could open up too many unwanted line matches, so not sure about that.

If it is limited to only checking comments and the start of the line, it seems like it wouldn't be too broad to me, but I understand your concern.

@todo in a doc block are not covered yet. This only check code standalone comments. It would be very good to have docblock @todo covered as they are potentially just as common as standalone comments

How do I fix this? Do I need to adjust what is returned by register()?

Thanks for the suggestions!

I can submit another patch once we clear up some of these questions.

1. You are right about not needing 'g'
2. Yes my updated regex does this :-)
3. Limiting a matching 'todo' without @ to be at the start of a line i.e. // todo or * todo would solve it, but that's probably adding a bit more complication to the regex. Maybe leave that discussion until the other things are done.
4. Adding T_DOC_COMMENT_TAG into register() will get you the tags, but then you will also need to get the subsequent comment string because $tokens[$stackPtr] will not have the actual comment text. So I guess a little bit of logic to say 'if we have matched T_DOC_COMMENT_TAG then get the subsequent comment text, or if matched T_COMMENT then use $tokens[$stackPtr]['content']

Also can I suggest that the warning text is changed slightly to @todo comments should be of the format "@todo Something" as this shows more clearly what the desired format should be.

Regarding point 4, not sure how much you have seen of the Coder source and methods, but to save you some time, in case you've not done much of it before,
$comment = $phpcsFile->findNext(T_DOC_COMMENT_STRING, ($stackPtr + 1))
will get you the comment text to check, after matching on T_DOC_COMMENT_TAG. Apologies if you know this already, but just trying to save you some effort. Also you do not need T_DOC_COMMENT_STRING in register()

This should be a nice sniff to add. I always forget how @todo should be formatted, so it will be good for me.

I'm new to writing coder sniffs, so any advice is appreciated.

1. Sounds good.
2. Sounds good.
3. Finding matches without @ is not how it works currently, and making it work like that seems to be trickier than I expected. So I'm inclined to leave this as is for the moment too.
4. Does this look right? I didn't test this yet. Wanted to see if I was on the right track first.

Do we want to check for indentation of multi-line @todo statements with this too? Perhaps that is a separate sniff?

Do you have a GitHub account? All Coder development is actually done on GitHub and via Pull Requests on https://github.com/pfrenssen/coder (see Coder project front page). On there, you can submit your changes, and I can review the specifc lines, with commenting etc. When complete the changes are mirrored back to drupal.org.

If you don't (or do not want to) we can continue here, but eventually your changes will need to be pushed to github as that is the route for updating the source, not direct to drupal.org.

Do you have a GitHub account?

Yes. adamzimmermann if you need to add me to anything.

You don't have to ask me twice to not deal with patches and use normal PR's! I'll get something submitted later today.

It appears I need to be granted permission to push a branch?

21:12:18-adamzimmermann~/www/coder (2222435-todo-formatting-sniff)$ git push origin -u 2222435-todo-formatting-sniff
ERROR: Permission to pfrenssen/coder.git denied to adamzimmermann.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I don't have permission either. For me, I just forked https://github.com/pfrenssen/coder into https://github.com/jonathan1055/coder then I push to my own branches. This also then tests on Travis https://travis-ci.org/github/jonathan1055/coder and when ready I create a PR - for example see https://github.com/pfrenssen/coder/pull/113 This seems to work nicely, and @Klausi and @Arkener like it.

Sounds good. Hoping to get to this later this week.

PR ready for review:

https://github.com/pfrenssen/coder/pull/118

I just finished integrating feedback that jonathan1055 helped me with and created a new PR that is ready for review. It even has tests ready to go and passing!

https://github.com/pfrenssen/coder/pull/120

Merged, thank you so much!

Please open a follow-up issue to create a fixer for this new sniff. Would be really helpful for many people if we could auto-fix @todo comments.

We could also open a follow-up to check the indentation of lines that follow an @todo comment.

Thanks for the review and commit. Yes in core the new sniff gives

--------------------------------------------------------------
A TOTAL OF 167 ERRORS AND 0 WARNINGS WERE FOUND IN 124 FILES
--------------------------------------------------------------

and we should definitely try to write an fixer.

@adamzimmermann do you want to do this? I'm happy to start it off if you don't have time right now.

I have raised #3177471: Create automatic fixer for @todo sniff

@jonathan1055 I'll give it a shot. Just assigned it to myself.

@klausi I created an issue for the trailing lines. #3177721: Create sniff for checking the indentation of lines that follow an @todo comment