Repository navigation
FIXME tags in list of Problems? #2364
Description
Hello everyone,
Is it possible to have the FIXME items in the list of problems, like the TODO ones?
I thought it had to come from the dart linter and the dart analysis server but I asked in dart-lang/sdk#57200 and I was redirected to an "IDE" solution.
Although I'm able to highlight the FIXMEs, they don't appear in the list of problems...
Also, I think that these should not be "INFO" like the TODO but WARNING maybe?
Today, I add TODO before FIXME to have them in the list but it's not really convenient...
Thanks!
Raphaël
VSCode 1.44.1
flutter doctor -v
[✓] Flutter (Channel master, v1.18.1-pre.20, on Linux, locale en_US.UTF-8)
• Flutter version 1.18.1-pre.20 at /opt/flutter
• Framework revision 981afe39ae (10 days ago), 2020-04-07 19:42:57 -0700
• Engine revision 394ac6b484
• Dart version 2.8.0 (build 2.8.0-dev.20.0 dcdc71d763)
[✓] Android toolchain - develop for Android devices (Android SDK version 28.0.3)
• Android SDK at /opt/android-sdk/
• Platform android-29, build-tools 28.0.3
• ANDROID_HOME = /opt/android-sdk
• ANDROID_SDK_ROOT = /opt/android-sdk
• Java binary at: /opt/android-studio/jre/bin/java
• Java version OpenJDK Runtime Environment (build 1.8.0_212-release-1586-b4-5784211)
• All Android licenses accepted.
[✓] Android Studio (version 3.6)
• Android Studio at /opt/android-studio
• Flutter plugin version 44.0.2
• Dart plugin version 192.7761
• Java version OpenJDK Runtime Environment (build 1.8.0_212-release-1586-b4-5784211)
Activity
DanTup commentedon Apr 20, 2020
The TODOs do come from the analysis server and there's no current way to customise them. From the link above it seems like IntelliJ does its own scanning for these, but in VS Code we just rely on them coming from the server.
@bwilkerson @pq is there any merit in having these customisable in the server (eg. a set of prefixes that you could customise the severity of) to avoid editors getting some from the server and using their own regex for others?
pq commentedon Apr 20, 2020
It sounds like a reasonable idea to me but I can't speak to the cost. Looking at
TodoFinder(where comments are scraped) it certainly seems doable. The more involved part is adding configuration support to the server protocol. @bwilkerson: thoughts?bwilkerson commentedon Apr 20, 2020
I don't know whether there's an industry standard for TODO-like comments, but if there is then we should probably support it without requiring customization. If there isn't, then some kind of customization is likely to be necessary (perhaps some metadata in the analysis options file). I do agree, though, that these should be coming from server and not from individual IDEs.
I am not opposed to having support for this. But I do have to raise the question of priority. Is this really the feature that will provide the greatest benefit to users for the developer and maintenance cost? I'm skeptical, but open to being proven wrong.
DanTup commentedon Apr 20, 2020
I haven't used many IDEs, but I remember Visual Studio allowed them to be customised (though it didn't show them in the Problems list, but its own window). The defaults it has are
HACK,NOTE,TODO,UNDONE,UnresolvedMergeConflict(!).I'm not sure it's a priority (I don't think it's come up much before), though it is something I've wished for in the past (sometimes I need to make "urgent TODOs"... things that I can't fix now, but must be done before merging a PR, for ex. - it would be nice if I could mark them so that they showed up in the editor, but also failed a step on my CI/PR builds as a reminder).
I think we could leave this issue open here a while to collect 👍 's to see if there's enough demand to make it worthwhile.
pq commentedon Apr 20, 2020
SGTM. Thanks @DanTup!
csells commentedon Aug 29, 2021
I'd like to see us implement more than just TODO. Can we start w/ some common ones, e.g. TODO, HACK, NOTE, FIXME, UNDONE w/o going all the way into letting people define their own?
bwilkerson commentedon Aug 29, 2021
Yes, we can do that. For TODO we support a couple of different styles:
Do we need to support multiple styles for the other markers or just the marker followed by a colon (such as "FIXME:")?
csells commentedon Aug 29, 2021
good catch. I think all three forms for the other common tags is ideal.
DanTup commentedon Aug 29, 2021
Are these all reasonable to categorise under the existing
TODOerror type/error code? Does the user need some way to enable them individually?I ask because
NOTEseems a bit vague if there's no way to opt-out - I suspect I've writtennote: blahafter many comments but wouldn't want them showing up in my Problems list even if I wanted TODOs.csells commentedon Aug 29, 2021
I think you're right, Danny. TODO, HACK, FIXME and UNDONE all fall into the category of "problem" and should be listed. We probably shouldn't add NOTE to this list so it doesn't show up under Problems. People can still use it, of course, but it'll be a, um, note and not a problem that needs fixing.
bwilkerson commentedon Aug 29, 2021
I'll also note that we don't currently support anything except the uppercase form of 'TODO'. You can't write 'todo' or 'Todo' or any other combination if you want it to be picked up.
csells commentedon Aug 29, 2021
the mixed and lower case versions seem like good alternatives for folks that don't want such notes to show up under Problems (in addition to the settings that Danny exposes allowing folks to turn that behavior on/off across an entire project).
DanTup commentedon Aug 29, 2021
Ah, if it's case-sensitive then maybe NOTE isn't so bad (although I do still wonder whether that's something people would expect to show up in the problems list?).
Is it reasonable to just consider them all TODOs (eg. there's only a single switch that turns them all on or off), or would they need to be toggled individually? (The first is easier, but it also means people with TODOs enabled will potentially see a lot more "Problems" afterwards).
bwilkerson commentedon Aug 29, 2021
I think it's fine if they're all controlled by a single switch. But I would recommend that we create a different error code for each marker so that clients can tell the difference in case they want to treat them differently in the future.
DanTup commentedon Aug 31, 2021
I made a start on this here:
https://dart-review.googlesource.com/c/sdk/+/212000/
A few questions:
todo, so these new ones arehack,fixme, etc. - is that reasonable or should they be more specific (liketodo.hacketc.)?TodoFinderassuming there doesn't need to be custom code for this (instead, the regex just hasTODOreplaced withTODO|HACK|FIXMEetc.)NOTE:should be included?I also extended the LSP/VS Code
dart.showTodosconfiguration to support not only a boolean, but aList<String>so that there's more fine-grain control (you can't just make your own up, but it lets you chose which of those we support to show). I noticed that my own projects have a lot ofHACK:comments that I wouldn't want showing up with TODOs because they're mostly descriptions of why some code is written in a less-than-ideal way, but usually required by some external library/API (I also noticed there are a few of them in the Flutter repo that aren't particularly actionable).csells commentedon Aug 31, 2021
I'm happy to include NOTE in the list if we're able to turn on/off each string independently. NOTE should default to off but be an option.
DanTup commentedon Aug 31, 2021
The setting mentioned above is specific to LSP. I don't know if it'd be trivial to toggle them individually for non-LSP clients though (I presume it would have to be done with analysis_options, but I'm not sure what the capabilities of that are or how difficult it is to change - @bwilkerson may know better).
With the currently LSP behaviour, using a boolean
dart.showTodos(which is the only option today) would show all TODO-type diagnostics. We could change this, but rather than having the inconsistency, I wonder if it would be better to leave NOTE and perhaps implement it if/when we support entirely custom tags (since then NOTE would just be a custom tag, rather than a built-in tag that defaults differently to the others)?csells commentedon Aug 31, 2021
Until we have the ability to turn off individual tags, I agree that leaving NOTE off the list would be better.
bwilkerson commentedon Aug 31, 2021
It's important that the unique name of an error code actually be unique, and we consistently use snake case for those name, but other than that it doesn't matter much. I'd probably just leave them unqualified.
That sounds good to me.
DanTup commentedon Sep 1, 2021
The existing
TodoCodeclass prefixed thenamewithTodoCode.for theuniqueName, so it looks like they would beTodoCode.TODO,TodoCode.HACK, etc.They show up lowercase in the frontend because somewhere (perhaps LSP) we're lowercasing all error codes (which is why I noted them as lowercase above).
I realised that there is an existing mechanism for hiding TODOs in analysis_options, and because it uses error codes we do actually have fine-grain control of these already (although people who have ignored
todowill start seeing the others until they hide them too):Although unlike the VS Code setting, you have to set that per-project (the VS Code/LSP setting you could set globally to apply to all projects).
Assuming the above is all ok, and we're ok with FIXME/HACK/UNDONE appearing in problems by default for existing users then I think https://dart-review.googlesource.com/c/sdk/+/212000/ should be good.
bwilkerson commentedon Sep 1, 2021
That was a deliberate choice. Our reasoning is that (a) it's more convenient for users to be able to type the lowercase forms, both in the analysis options file and in ignore comments, and (b) it looks better in error output. I still believe that it's the right choice.
[analysis_server] Support HACK, FIXME, UNDONE types for TODO comments
DanTup commentedon Sep 1, 2021
Yep, I agree with that choice (my comment was just rambling rather than questioning 😄).
This landed in dart-lang/sdk@681243f so they'll start showing up in a future SDK release (and can be toggled using the analysis_options syntax shown above, or with the VS Code
dart.showTodossetting once #3547 is done).