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 textDocument/prepareRename request #30

Merged
merged 3 commits into from Aug 6, 2022

Conversation

artempyanykh
Copy link
Contributor

@artempyanykh artempyanykh commented Aug 6, 2022

prepareRename with a range response allows rename to work reasonably with Markdown symbols that can contain whitespace and other kinds of punctuation. Also, makes rename behave better on invalid positions (instead of new name prompt followed by a no-op rename the user gets 'no rename' and no prompt).

Description in LSP spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_prepareRename

NOTE: the type of response is not modelled fully as it's a bit involved. I hope supporting additional response shapes
can be done in a follow up if needed.
@artempyanykh
Copy link
Contributor Author

artempyanykh commented Aug 6, 2022

The default behaviour is incorrect. I'll fix it and update the PR. Fixed.

notImplemented cannot be used because the server will fail on rename.
None cannot be used because it marks an invalid rename.

We need to respond with a {defaultBehavior=true} so that those servers
that didn't override prepareRename still have the same rename behavior
as before.

NOTE: I needed to model the response fully to make things work.
@baronfel
Copy link
Contributor

baronfel commented Aug 6, 2022

Nice! We really do need a better way to handle things like WorkDoneProgressParams, but this is a general failing of this library at the moment.

@baronfel baronfel merged commit 078ec02 into ionide:main Aug 6, 2022
@artempyanykh artempyanykh deleted the prepareRename branch August 6, 2022 14:59
artempyanykh added a commit to artempyanykh/LanguageServerProtocol that referenced this pull request Aug 7, 2022
This is a continuation to ionide#30. Apparently, nvim and emacs treat rename
capabilities a bit differently which is why I didn't notice the problem
before I tried using Marksman with Emacs.
baronfel pushed a commit that referenced this pull request Aug 7, 2022
This is a continuation to #30. Apparently, nvim and emacs treat rename
capabilities a bit differently which is why I didn't notice the problem
before I tried using Marksman with Emacs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants