Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIXME tags in list of Problems? #2364

Closed
rdehouss opened this issue Apr 18, 2020 · 22 comments
Closed

FIXME tags in list of Problems? #2364

rdehouss opened this issue Apr 18, 2020 · 22 comments
Labels
in editor Relates to code editing or language features is enhancement
Milestone

Comments

@rdehouss
Copy link

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/linter#51 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...
image

image

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)
@DanTup
Copy link
Member

DanTup commented 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
Copy link

pq commented 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
Copy link

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
Copy link
Member

DanTup commented 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.

@DanTup DanTup added this to the Backlog milestone Apr 20, 2020
@DanTup DanTup added in editor Relates to code editing or language features is enhancement labels Apr 20, 2020
@pq
Copy link

pq commented Apr 20, 2020

SGTM. Thanks @DanTup!

@csells
Copy link

csells commented 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
Copy link

Yes, we can do that. For TODO we support a couple of different styles:

  • TODO
  • TODO:
  • TODO(username)

Do we need to support multiple styles for the other markers or just the marker followed by a colon (such as "FIXME:")?

@csells
Copy link

csells commented Aug 29, 2021

good catch. I think all three forms for the other common tags is ideal.

@DanTup
Copy link
Member

DanTup commented Aug 29, 2021

Are these all reasonable to categorise under the existing TODO error type/error code? Does the user need some way to enable them individually?

I ask because NOTE seems a bit vague if there's no way to opt-out - I suspect I've written note: blah after many comments but wouldn't want them showing up in my Problems list even if I wanted TODOs.

@csells
Copy link

csells commented 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
Copy link

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
Copy link

csells commented 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
Copy link
Member

DanTup commented 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
Copy link

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
Copy link
Member

DanTup commented Aug 31, 2021

I made a start on this here:

https://dart-review.googlesource.com/c/sdk/+/212000/

A few questions:

  • The todo error code is just todo, so these new ones are hack, fixme, etc. - is that reasonable or should they be more specific (like todo.hack etc.)?
  • I just extended the existing TodoFinder assuming there doesn't need to be custom code for this (instead, the regex just has TODO replaced with TODO|HACK|FIXME etc.)
  • Did we conclude NOTE: should be included?

I also extended the LSP/VS Code dart.showTodos configuration to support not only a boolean, but a List<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 of HACK: 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
Copy link

csells commented 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
Copy link
Member

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

NOTE should default to off but be an option.

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
Copy link

csells commented 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
Copy link

The todo error code is just todo, so these new ones are hack, fixme, etc. - is that reasonable or should they be more specific (like todo.hack etc.)?

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.

I just extended the existing TodoFinder assuming there doesn't need to be custom code for this (instead, the regex just has TODO replaced with TODO|HACK|FIXME etc.)

That sounds good to me.

@DanTup
Copy link
Member

DanTup commented Sep 1, 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.

The existing TodoCode class prefixed the name with TodoCode. for the uniqueName, so it looks like they would be TodoCode.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 todo will start seeing the others until they hide them too):

Screenshot 2021-09-01 at 11 11 55

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
Copy link

They show up lowercase in the frontend because somewhere (perhaps LSP) we're lowercasing all error codes ...

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.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Sep 1, 2021
Fixes Dart-Code/Dart-Code#2364.

Change-Id: I0c66e6443e9a1b0bb3795b6db29de81e43e52b61
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212000
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
@DanTup
Copy link
Member

DanTup commented 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.showTodos setting once #3547 is done).

@DanTup DanTup closed this as completed Sep 1, 2021
@DanTup DanTup modified the milestones: Backlog, v3.27.0 Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features is enhancement
Projects
None yet
Development

No branches or pull requests

5 participants