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

chore(lsp): check rename capabilities before send rename action #2203

Merged
merged 1 commit into from
Apr 23, 2022

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Apr 20, 2022

check rename capabilities before send rename action

and logging a kindly warning message to the log file so that the user knows that it maybe a client config problem.

some lang server implementation (like intelephense) need a license key configured to do rename action.

in neovim, the config looks like:

lsp.intelephense.setup {
	settings = {
		intelephense = {
		        licenceKey = "the-actual-license-key",
		},
	},
}

in helix, one may miss configured like this:

in languages.toml

[[language]]
name = "php"
config = { "intelephense" = { "licenceKey" = "the-actual-license-key" }}

but the correct init config in helix should be like this:

[[language]]
name = "php"
config = { "licenceKey" = "the-actual-license-key" }

with the warning message in the log, people will easy to debug and confirm the config issues.

@ttys3 ttys3 force-pushed the check-rename-cap branch from 01d9ccb to 15d2aad Compare April 20, 2022 18:54
@ttys3 ttys3 marked this pull request as draft April 20, 2022 19:08
Comment on lines 871 to 872
log::warn!("the server does not support rename");
return Ok(lsp::WorkspaceEdit::default());
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to show an editor error with editor.set_error in the calling code if it returns an Err variant from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks point out, after pushed, I also found this and updating now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested ok:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated, PTAL

@ttys3 ttys3 force-pushed the check-rename-cap branch from 15d2aad to 63b3185 Compare April 20, 2022 19:22
@ttys3 ttys3 marked this pull request as ready for review April 20, 2022 19:35
@ttys3 ttys3 requested a review from sudormrfbin April 20, 2022 19:35
@David-Else
Copy link
Contributor

Cool! Also, some language servers don't have rename yet, like the bash one.

@archseer archseer merged commit 19d042d into helix-editor:master Apr 23, 2022
@ttys3 ttys3 deleted the check-rename-cap branch April 23, 2022 16:54
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.

4 participants