-
Notifications
You must be signed in to change notification settings - Fork 812
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
Add window/progress #245
Add window/progress #245
Conversation
@keegancsmith, It will cover your contributions to all Microsoft-managed open source projects. |
@keegancsmith, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
@keegancsmith thanks for the PR. Actually adding this needs to be done under considering how other tools might implement something like this. The LSP client side is implemented in Eclipse, GNome builder, ... I will mark this one as a dup of #70 which already contains quite some discussions around this topic. |
@felixfbecker Do you think we could introduce this simple notification for starters, see how it goes and refine from there? I believe standarizing a simple progress protocol would incentivize more clients to implement at least some form of reporting the progress. Currently the Rust Language Server uses custom |
Sure, I think if multiple servers or clients want this it is reasonable to implement as an experimental/extension method and see if it picks up, then repropose it again. The way we did this with extensions at Sourcegraph was to prefix the methods with Is the PR in it's current form sufficient for your needs? Any other suggestions? |
Regarding the current form, I think a more practical/consistent approach would be: /**
- * The title of the progress.
- * This should be the same for all ProgressParams with the same id.
+ * Optional title of the progress.
+ * If unset, the previous title (if any) is still valid.
*/
- title: string;
+ title?: string; Seems redundant and/or hard to enforce passing the same title for a given id, but with the above change it'd be possible not to set any without introducing a new message - something I'm not sure is desired. I think it's simple and self-contained enough in the current form as-is, I think it'd be nice to introduce it as an initial/experimental version. @dbaeumer what are your thoughts? |
That makes a lot of sense to me. Wanna do a PR to the sourcegraph repo? We can have this live there as an unofficial spec extension for the time being |
Proposing this as of microsoft#245 (comment). Existing discussion regarding the title: sourcegraph#21 (comment).
Proposing this as of microsoft#245 (comment). Existing discussion regarding the title: #21 (comment).
I added my comments here: microsoft/vscode-languageserver-node#261 (comment). I am open to where the discussion should happen but since a lively discussion happened in the PR I thought it is a good place to have it. |
VS Code 1.12 includes an API for progress:
This proposal intends to make it possible for LSP to provide similar functionality.
The progress extension in vscode allows logging to source control and the window. However, we do not include that ability in this spec since: