Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Incorrect:
// @todo: This is something I should not forgot to do.
// @todo - this is something I should not forgot to do.
// @todo this is something I should not forgot to do.
// TODO: This is something I should not forgot to do.
Correct (https://www.drupal.org/node/1354#todo):
// @todo This is something I should not forgot to do.
Also, @todo comments may also appear in docblocks and this is currently not flagged as an error:
/**
* Implements hook_nodeapi().
*
* @todo: move this to blah which handles all the other domain
* related stuff. Too much refactoring for right now though.
*/
Problems with this:
- It should be
@todo Move...
- There is a double space between sentences
- The comment could be reflowed to 80 chars as more would fit in the first line.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2222435-18-todo-comment-sniff.patch | 2.08 KB | adamzimmermann |
#11 | 2222435-11-todo-comment-sniff.patch | 1.83 KB | adamzimmermann |
Comments
Comment #1
psynaptic CreditAttribution: psynaptic commentedComment #2
psynaptic CreditAttribution: psynaptic commentedComment #3
psynaptic CreditAttribution: psynaptic commentedComment #4
psynaptic CreditAttribution: psynaptic commentedComment #5
Elijah LynnBig +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:.
Comment #6
Elijah LynnAdd summary link to @todo standard documentation, here => https://www.drupal.org/node/1354#todo.
Comment #7
dakku CreditAttribution: dakku as a volunteer commented+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.
Comment #8
seanBSomething I just noticed. The following code returns an error, even though it is following the coding standards:
26 | ERROR | Comment indentation error, expected only 1 spaces
The code below passes when it shouldn't:
Comment #9
Watergate CreditAttribution: Watergate at Sicse commentedCoding 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
.Comment #10
apadernoComment #11
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedI 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.Comment #12
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented@adamzimmermann This looks great. I have tested the patch on real code, and in principle it works. Just noted a few things:
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]
Comment #13
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI have updated the regex with some suggestions - see https://regex101.com/r/cir1XP/3
@to do
@to- do
is also reported\s\w
after the final @todo so that only spaces are allowed before the text, i.e. no : or -Hope it's OK that I updated your regex links. Thanks.
Comment #14
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedI assumed I didn't need it since it was being fed things line by line, or that was my assumption. Perhaps that is incorrect?
Good call. Sounds like you just fixed this!
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.
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.
Comment #15
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented// 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.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.Comment #16
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedRegarding 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.
Comment #17
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedI'm new to writing coder sniffs, so any advice is appreciated.
Comment #18
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented@
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.Do we want to check for indentation of multi-line @todo statements with this too? Perhaps that is a separate sniff?
Comment #19
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedDo 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.
Comment #20
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedYes.
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.
Comment #21
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedIt appears I need to be granted permission to push a branch?
Comment #22
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI 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.
Comment #23
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedSounds good. Hoping to get to this later this week.
Comment #24
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedPR ready for review:
https://github.com/pfrenssen/coder/pull/118
Comment #25
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commentedI 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
Comment #27
klausiMerged, 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.
Comment #28
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks for the review and commit. Yes in core the new sniff gives
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.
Comment #29
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI have raised #3177471: Create automatic fixer for @todo sniff
Comment #30
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented@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
Comment #31
adamzimmermann CreditAttribution: adamzimmermann at Chromatic commented