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

Implement LSP didDeleteFiles to clear diagnostics #47553

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

Razoric480
Copy link
Contributor

As the LSP specifications require, the language server is responsible for ownership of diagnostics. At the moment, when a file is deleted, nothing happens so errors persist even though they no longer exist.

Brings in a feature from the LSP 3.16.0 specifications for detecting file operations, namely didDeleteFiles, in the workspace. Clients that are not up to date to 3.16.0 spec will need to be updated to make use of it, but otherwise can be ignored by those that don't without issue.

Fixes #43133

@Razoric480 Razoric480 requested a review from a team as a code owner April 1, 2021 17:44
@Calinou Calinou added this to the 3.4 milestone Apr 1, 2021
@akien-mga akien-mga merged commit e6e1f62 into godotengine:3.x Apr 1, 2021
@akien-mga
Copy link
Member

Ah I didn't see that this targeted the 3.x branch and not master :( should likely have waited before merging it then as we're very close to the 3.3 release. Should hopefully be fine for 3.3 as it doesn't seem to impact the rest of the LSP stuff.

Please make PRs against master in priority, or if you need to work on 3.x first, make it explicit in the description and state what you intend to do for master.

@Razoric480
Copy link
Contributor Author

Vnen has stated that he's aiming to tackle LSP issues with the new GDScript implementation in time for Godot 4.0, and to focus on fixes that go towards 3.x. I should have added that as a clarification in the PR notes - sorry.

@akien-mga
Copy link
Member

CC @vnen. IMO there's no reason not to do the same fixes to master if the code is still the same - if/when the LSP is being reworked, it's good if all things are up-to-date, to avoid missing stuff like this.

@NathanLovato
Copy link
Contributor

NathanLovato commented Apr 2, 2021

Makes sense, let's see what George says.

It's my bad as I talked with vnen about hiring Razoric to address remaining issues and was the one who instructed François to do it in version 3.

I guess it could be like what others do: same PR to master and 3.X or the cherry-pick label. Whatever works best for you!

@vnen
Copy link
Member

vnen commented Apr 6, 2021

I mean, I do intend to fix LSP issues when it comes time to do bugfixing, but of course other people can contribute to it before I do. The code is pretty much the same so it should be easy to the changes in both 3.x and master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants