Skip to content
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 proposed 'window/progress' notification #103

Merged
merged 3 commits into from
Apr 15, 2019

Conversation

Xanewok
Copy link
Collaborator

@Xanewok Xanewok commented Apr 12, 2019

Closes #43.

Sorry, I'm too itchy to not open it - the original proposed PR has been opened 1.5 years ago so it's about time 😀

This adds proposed feature which enables proposed LSP extensions.

RLS PR that's working and which only reexports the types - Xanewok/rls@68da767

@@ -59,6 +59,10 @@ macro_rules! lsp_notification {
("workspace/didChangeWorkspaceFolders") => {
$crate::notification::DidChangeWorkspaceFolders
};
// Proposed
// ("window/progress") => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I wanted to warn the user if they're trying to use a feature that's proposed but since the macro has to return specifically a type, I wasn't sure what to return here, so I just commented it out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine to just return the type as normal, if proposed is not enabled then it will just be a compile error. (Add a comment on it so people can at least read to source to see why it does not work).

src/notification.rs Outdated Show resolved Hide resolved
@@ -59,6 +59,10 @@ macro_rules! lsp_notification {
("workspace/didChangeWorkspaceFolders") => {
$crate::notification::DidChangeWorkspaceFolders
};
// Proposed
// ("window/progress") => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine to just return the type as normal, if proposed is not enabled then it will just be a compile error. (Add a comment on it so people can at least read to source to see why it does not work).

src/notification.rs Outdated Show resolved Hide resolved
src/notification.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@Marwes
Copy link
Member

Marwes commented Apr 12, 2019

What is left after microsoft/vscode-languageserver-node#261 to make it properly part of the spec?

@Xanewok
Copy link
Collaborator Author

Xanewok commented Apr 12, 2019

Done, addressed the feedback. While modifying .travis.yml it made sense to test also stable since this doesn't require any nightly features - should I remove it from the matrix?

What is left after microsoft/vscode-languageserver-node#261 to make it properly part of the spec?

That's... actually a good question. So the PR is proposed and a reference implementation is provided as per their recommendation. I imagine the proposed features will make it in next LSP version (3.15?) so that the users can start using it.

@Xanewok
Copy link
Collaborator Author

Xanewok commented Apr 14, 2019

The checks are all green now. I also added stable support (since we're not using any nightly features) - is that okay?

@Marwes Marwes merged commit d01bed5 into gluon-lang:master Apr 15, 2019
@Xanewok Xanewok deleted the window-progress branch April 15, 2019 07:22
@Marwes
Copy link
Member

Marwes commented Apr 15, 2019

Released as 0.57.

@Xanewok
Copy link
Collaborator Author

Xanewok commented Apr 15, 2019

Thanks! 🎉

Xanewok added a commit to Xanewok/rls that referenced this pull request Apr 15, 2019
@Xanewok
Copy link
Collaborator Author

Xanewok commented Apr 17, 2019

For better or worse, this was further split into 4 different message literals - microsoft/language-server-protocol#70 (comment)

@Marwes
Copy link
Member

Marwes commented Apr 17, 2019

Whelp, I guess this still works until it gets properly stabilized though?

@Xanewok
Copy link
Collaborator Author

Xanewok commented Apr 17, 2019

Yeah, it does; this was a proposed extension by Facebook et al. and couple of servers adopted this window/progress version, so it's useful on its own. When the current version is stabilized we should update the types here.

Xanewok added a commit to Xanewok/rls that referenced this pull request Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants