dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.95k stars 4.02k forks source link

Should the comment TODO behavior be changed? #57899

Open CyrusNajmabadi opened 2 years ago

CyrusNajmabadi commented 2 years ago

Discussed in https://github.com/dotnet/roslyn/discussions/57898

Originally posted by **Korporal** November 20, 2021 I've been writing TODO comments more recently as reminders to revisit certain areas of code that might require refinement. I found that if I write something like this: ```cs // TODO we must do some stuff in the future here because // we can't be certain in all cases that what we assumed at // the outset is still applicable partway through. ``` or ```cs /* TODO we must do some stuff in the future here because we can't be certain in all cases that what we assumed at the outset is still applicable partway through. */ ``` then the task list window only show the comment text between the opening of the TODO comment and the next end of line. I'd find it more natural if either the entire comment block was printed in the task list or all of the text up to and including the next period. Admittedly this is a small issue but it would improve the experience for me when I have a lot of these comments, when written in a hurry (as some often are) the user might not be conscious of this end of line behavior and might just not see certain parts of important TODO comments weeks later when scanning the task list for these.
BhaaLseN commented 2 years ago

I've wondered about this as well in the past, but fortunately we don't see this too often.

But when we do, things look like this (note that we also have other keywords like "FIXME" etc. in the Task List settings, so this probably shouldn't just apply to "TODO" comments):

// TODO JIRA-1234: for compatibility reasons, we do this [something that hits a long line limit]
//                 [where we break it up so it reads nicer, especially in diffs etc.]
//                 ...and the text is aligned to the first actual todo column;
//                 which should likely be trimmed off in the Task List.

or:

// FIXME: this needs to be [some longer description again]
//        [which is also aligned with the text]

And a few ugly ones that feel like they could use less space:

// TODO
// do the thing.

(which could've just been a simple // TODO: do the thing. to make it read nicer on the Task List, because right now it simply shows "TODO" on there)

All of those would benefit from being parsed until some arbitrary limit of lines/characters/whatever (mostly to avoid dumping a whole code file into the Task List when somebody thought it was a good idea to comment out a class and leave a // TODO comment in front), probably only during continous comment blocks (stopping at gaps where no leading // is present), with whitespace collapsed (so it shows legible text rather than chunks of whitespace between words)

The most common ones are single line though (and text on the same line as the keyword).

And for reference (because I found an actual stray one), there are cases where stupid things might happen:

// TODO
//if (someCondition)
//    CallAnotherMethod();

(which is silly, both the "TODO" that doesn't say what needs to be done and the commented out code; and shouldn't exist in first place)

pinkfloydx33 commented 2 years ago

I've been adding TODO on each line of block/multi-line Todo comments. It works, in that each line shows up in the task list, but then it looks a little confusing. So I started changing them to TODO [cont] just to provide a level of disambiguation. I'd definitely appreciate something like this.

I'm wondering if it's better (or easier, or both?) to perhaps do this only for block comments and just assume the whole block (beginning with the keyword) is the task. An alternative would be recognizing the word "TODO" somewhere in the middle of the text and flagging the entire block.

Or maybe some sort of sentinel that's more than just a basic period/full stop. Something like /TODO or END TODO which could be extended to other keywords like the above mentioned FIXME

jmarolf commented 2 years ago

The error list has the ability to show more info for a given item by expanding it. This is what we do for warnings like AD0001 where we show the stack trace. Considering that the Task List in Visual Studio is based off the same control I think we should just opt-in to this functionality.

Essentially the Task List would look the same, but you could choose to expand an item to see the entire comment if you wanted.

Cosifne commented 2 years ago

Design meeting note: We need to talk to platform team about the support & APIs for TODO comments