-
Notifications
You must be signed in to change notification settings - Fork 340
filter todos from the problems view #38
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
Conversation
devoncarew
commented
Aug 6, 2016
- filter todos from the problems view (and add a preferences for it)
- add a hidden flag to start the analysis server diagnostic server up on a specified port
| }, | ||
| "dart.showTodos": { | ||
| "type": "boolean", | ||
| "default": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly torn about this default... It could be quite noisy if you have a lot, but it's also a nice reminder you should deal with this stuff. If we default to off I suspect most people will never know the option exists, but without a way to filter it in the Problems window maybe it'll just be more annoying than anything.
I just Google'd to see if I can find what other languages do.. Didn't find any evidence of any showing TODOs, but I did find this (watch the animated GIF) which looks quite cool (but doesn't seem like something a Dart-specific extension would be expected to do!)
https://github.com/MattiasPernhult/vscode-todo
Let's leave as is for now and hope maybe we'll get a filter or something in future. I guess the file-search is a reasonable substitute if you do want to go and fix them (which I really need to on this codebase already!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave as is for now
I think you're right about the discovery issue - people won't know to turn it on.
That todo extension looks nice. It does seem like you want to work with todos occasionally, and not all the time like errors or warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about collapsing the TODOs into a single line?
You have 123 TODOs. Set dart.showTodos in settings to have them displayed here
I think having one item in the list won't be a problem and it aids discovery. If we can hook the user clicking on it (I suspect not) we could even do "click to show" and then fill them in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how well this'd work - we have to give Code a Uri for a set. It might not accept a null or fake filename; possibly could attribute them against pubspec or similar, but then it's starting to feel a bit hacky :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm leaning towards enabling it by default, and assuming that people will see the setting if they want to disable it.
Right now it takes a restart before the setting really takes effect - it filters as the analysis errors are created, and doesn't remember them to re-report to vscode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy for the default to be on until we have a better option (and like you say, people more likely to look for an option if they don't like it). Not applying immediately sucks a little, but we could send an analysis.reanalyze from vscode.workspace.onDidChangeConfiguration? (though it looks like we don't get told which values changed; we'd have to always do it of stash a copy of the previous value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to reanalyzing if that setting changes, it'll be nice to not have the UI
out of sync with the pref setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds like default=on and reanalyze on change seems best for now. I've updated #39 for handling this.
|
Merging this in because I think it's all good. I'll raise an issue about investigating options for showing TODOs. |