Show analysis status in status bar (+ other tweaks) #42
Conversation
| private handleErrors(notification: as.AnalysisErrorsNotification) { | ||
| let errors = notification.errors; | ||
| if (!getConfig('showTodos')) | ||
| if (!getConfig<boolean>('showTodos')) |
There was a problem hiding this comment.
I can't remember why, I tried to standardise on double quotes for strings. I seemed to be randomly mixing them and it kinda bugged me. Maybe I thought I'd be putting apostrophes in English strings more and I guess they're also slightly more distinguishable from template-strings (backticks).
You don't have to worry about changing this but I thought it worth mentioning (or maybe there's a reason I should switch!?)
There was a problem hiding this comment.
I'd also been putting config names into constants; though currently they're littered around the place and probably should go together somewhere; I'll probably pull them all together soon into a strongly-typed class.
There was a problem hiding this comment.
I'll probably pull them all together soon into a strongly-typed class.
Sounds great - I have the same string literal (showTodos) in 2-3 places now; having a class for the pref name would be more robust.
There was a problem hiding this comment.
Yep; just working on this now.. Looks something like this:
import { config } from "./config";
if (config.showTodos):)
There was a problem hiding this comment.
I also moved verbose to config (hidden). This way you can turn on and off without a recompile, it applies immediately. It's off by default (since hiddens don't have defaults) but that seems reasonable.
There was a problem hiding this comment.
it applies immediately
ah, that's very nice. the analysis server output can be very useful but high volume. It's great to be able to toggle it live.
|
Looks great! I've added a couple of minor comments. If I get some enough time today I might merge this in and tweak those things myself; would be nice to get another build out with all the changes! Minor nitpick: I tend to prefer smaller individual commits for functionally different things (easier to describe them in the commit message and to attribute them to different issues). Don't feel that strongly about it (esp for such small changesets) but thought it worth noting. Maybe I'll add some notes to the contributing file. Thanks again; really appreciate the effort (and it's great to have an experienced Dart dev's input, I'm still a Dart noob). I'm also really keen on a good IDE integration; I've really not got on with the other IDEs (and bad IDE support was actually one of the big factors in us not adopting Dart at work).. I was sad when CDE was abandoned (I still am a little, since Code won't run on ChromeOS!). |
Totally makes sense. I generally try and be hygienic w/ my PRs and just send one per topic. Easy enough to do going forward :) |
|
Thanks :) I've tweaked some stuff based on my comments above, believe everything mentioned has been done in this PR. Thanks again! |
Analyzing...) in the status line when the analysis server is busyserverShutdown- if I understand correctly it's basically a no-op@DanTup, it's a grab bag of work. Probably it for this weekend! I'm excited that Dart's getting a plugin for vscode :)