Skip to content

Conversation

@devoncarew
Copy link
Contributor

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

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!).

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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 :(

Copy link
Contributor Author

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.

Copy link
Member

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)

Copy link
Contributor Author

@devoncarew devoncarew Aug 6, 2016

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

Copy link
Member

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.

@DanTup
Copy link
Member

DanTup commented Aug 6, 2016

Merging this in because I think it's all good. I'll raise an issue about investigating options for showing TODOs.

@DanTup DanTup merged commit f1766a5 into Dart-Code:master Aug 6, 2016
@DanTup DanTup added this to the v0.6 milestone Aug 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants