accept window/progress instead of beginBuild/diagnosticEnd #231
Conversation
algesten
force-pushed
the
progress-params
branch
2 times, most recently
from
cf399c8 to
85d1c14
Compare
|
Moved spinner order that it reads |
src/extension.ts
Outdated
| if (Object.keys(runningProgress).length) { | ||
| const status = | ||
| typeof progress.percentage === 'number' ? asPercent(progress.percentage) : | ||
| progress.message ? progress.message : |
There was a problem hiding this comment.
Title is ignored in favour of message, if it's specified - is that intended?
This leads to RLS * [build] followed by RLS * crate_name when building, where we lose information that we're building that crate.
There was a problem hiding this comment.
@Xanewok This is indeed intended. I wanted to keep the information flow at the bottom terse, and it's mostly obviously what it's doing. The [<title>] is to get something rather than RLS *, but could maybe dropped.
It's kinda interesting to see how long it spends doing [diagnostics] though. Already now I can see we should look into performance here.
|
One thing I wondered is whether we should make the plugin in an interim handle both |
|
Since the user can specify the requested toolchain via the extension, I think there's no way around this other than just adding it in a backwards-compatibility fashion. |
|
I fix |
algesten
force-pushed
the
progress-params
branch
from
85d1c14 to
cebbfb2
Compare
algesten
force-pushed
the
progress-params
branch
from
cebbfb2 to
ed6896c
Compare
src/extension.ts
Outdated
| } | ||
| if (Object.keys(runningProgress).length) { | ||
| const status = | ||
| typeof progress.percentage === 'number' ? asPercent(progress.percentage) : |
There was a problem hiding this comment.
Can you use an if statment rather than these nested ternary operators, it's pretty hard to read as is.
|
@nrc done |
Xanewok
left a comment
Xanewok
left a comment
There was a problem hiding this comment.
I'd explore if it'd be feasible to use Set here to simplify changes made here a bit, but apart from that LGTM!
Sorry for making you wait so long! I knew I forgot about some r? review.
src/extension.ts
Outdated
|
|
||
| function diagnosticCounter() { | ||
| function progressCounter() { | ||
| const runningProgress: { [index: string]: boolean } = {}; |
There was a problem hiding this comment.
I'm really wonky with JS, but would it be possible to replace that with Set for clarity?
Also not really sure why/how it's allowed to be const, since it's clearly modified in the callbacks, but I guess that's TypeScript shenanigans that I'm missing.
There was a problem hiding this comment.
Agreed. Set is better.
const is not typescript shenanigans but javascript. It's not actually immutable (sadly), it's just the actual variable itself that is impossible to change. I.e.
const foo = 42;
foo = 43; // FAIL
There was a problem hiding this comment.
Right, makes sense. Thanks for the changes!
src/extension.ts
Outdated
| } else { | ||
| runningProgress[progress.id] = true; | ||
| } | ||
| if (Object.keys(runningProgress).length) { |
There was a problem hiding this comment.
If Set is used, these can be simplified (as per above)
| } else if (progress.message) { | ||
| status = progress.message; | ||
| } else if (progress.title) { | ||
| status = `[${progress.title.toLowerCase()}]`; |
There was a problem hiding this comment.
nit: I know it's up to clients to display this on the UI side, but shouldn't we just display it how servers send it?
There was a problem hiding this comment.
Ultimately I don't really want to end up in this part of the logic. This is a last resort fallback. We should try to send percentages or messages instead. It looked really bad to me with the caps, but this just taste and style, so I'm only going to argue my case once :)
If you think it's an important point, I'll change it.
|
Let's land this :) r? @Xanewok |
|
I just double checked that latest rls master works with the changes here. All good. |
|
Merged, thanks for working on this! |
* stubbed out separate test projects for actual tests * added Cargo.lock files
3 participants
This is the vscode side of rust-lang/rls#653