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

refactor(lsp): mark server_ready function as deprecated #23520

Merged
merged 1 commit into from
May 13, 2023

Conversation

glepnir
Copy link
Member

@glepnir glepnir commented May 7, 2023

window/progress not part of lsp spec. we shouldn't have this handler. Consider it already used in some user config or plugin. So mark as deprecated and remove in next version

Fix #23511

@github-actions github-actions bot added lsp refactor changes that are not features or bugfixes labels May 7, 2023
@clason
Copy link
Member

clason commented May 7, 2023

This may be throwing out the baby with the bath water. Presumably, plugins need or at least want a "server ready" function. Just because this request is fake doesn't mean we can't use another, valid, request, or some other way of checking whether a server is ready to answer requests.

@glepnir
Copy link
Member Author

glepnir commented May 7, 2023

this notify does not exist in lsp spec. you can't find it on spec. don't know why we keep this handler?

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/

@clason
Copy link
Member

clason commented May 7, 2023

Again, it's about the server_ready function, not the specific window/progress request. We can keep the function and just use a different, valid, request.

(To be clear, this is not a handler for a message. This is a utility function that sends a message -- but that is an implementation detail that can (and should) be changed.)

@glepnir
Copy link
Member Author

glepnir commented May 7, 2023

there has $/progress in lsp spec. if want make sure the server ready why don't use this ?

@clason
Copy link
Member

clason commented May 7, 2023

So this is just a typo, or someone forgot to adapt to changes in the spec. Why not just fix that then?

@glepnir
Copy link
Member Author

glepnir commented May 7, 2023

upport server progress notifications via $/progress, removing support for the non-standard window/progress (#545).

a server update change log. and this notify looks like never exist in lsp spec or only in 2018? idk just search it only on some old codes like rls. I check some servers source code they don't handle this notify. so this mean if you call server ready an error in your log.

Things that are not standard should not remain in neovim core. Anyone who needs to call this function should know what it's doing. If you confirm whether the server is ready, you should use the progress provided by the specification, right?

feel free to close or merge . because I never used this 😃

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

Given that there really is no good alternative in the specification to detect if a server is ready I'm inclined to merge this. If my interpretation of the spec is correct, after the initialize handshake a server should be ready to serve requests.

I think what's an issue with many language servers is if they've finished analysing the workspace, but given that edits can always re-trigger analysis/builds that's always a catch-up game, and it's also unclear what an editor should do if the server is still processing changes. It is also not a simple ready/not-ready, because depending on the implementations they can serve some requests with sensible results, some with outdated, some not at all.

I'll wait a bit longer before merge to give others a chance to comment.

@mfussenegger mfussenegger merged commit 512a905 into neovim:master May 13, 2023
12 of 13 checks passed
@github-actions github-actions bot removed the request for review from folke May 13, 2023 09:27
@A-Lamia
Copy link

A-Lamia commented May 17, 2023

I was using that function to check if LSP's where attached so i could run some autocommands, what would be a good replacement for this now ?

@clason
Copy link
Member

clason commented May 17, 2023

The LspAttach event.

@A-Lamia
Copy link

A-Lamia commented May 17, 2023

Isn't that only used to run the command on attach ? I was using it more as a check on ModeChange.

EDIT: dw just looked through docs and came up with an other way i guess ty.

folke pushed a commit to folke/neovim that referenced this pull request May 22, 2023
Julian added a commit to Julian/lean.nvim that referenced this pull request Jun 17, 2023
It's deprecated and won't appear in neovim 0.10.

See neovim/neovim#23520

We're trying to make sure we get a response here, so just
call some other LSP method and see, though we may be able to
remove this entirely.
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
@justinmk justinmk mentioned this pull request Jun 19, 2023
2 tasks
@justinmk
Copy link
Member

Documented in #24061

justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
justinmk added a commit to justinmk/neovim that referenced this pull request Jun 19, 2023
justinmk added a commit that referenced this pull request Jun 19, 2023
- nvim requires rpc responses in reverse order. #19932
- NVIM_APPNAME: UIs normally should NOT set this.

ref #23520
fix #24050
fix #23660
fix #23353
fix #23337
fix #22213
fix #19161
fix #18088
fix #20693
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp refactor changes that are not features or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Neovim exposes an out of spec 'window/progress' lsp notification api
5 participants