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

Change default LSP Port for Godot 4's one #469

Closed

Conversation

jpcerrone
Copy link

@jpcerrone jpcerrone commented Apr 27, 2023

Changes the default LSP Port from 6008 (Used in Godot 3) for 6005 (Used in Godot 4). This change was made in godotengine/godot@de7873c .

Also fixes a potential bug in the check_client_status() function. It was wrongly getting the serverProperty from the serverPort configuration and viceversa.

@xqrdot
Copy link

xqrdot commented May 1, 2023

That would break the compatibility with Godot 3, which is an LTS version of Godot as of now. Ideally, automatic LSP port discovery should be a thing, with "6008" and "6005" being default options.

I'd suggest making a separate PR to fix the check_client_status() method: that fix can easily make its way to an upstream.

@paddy-exe paddy-exe added bug language server Issue is not originating from this extension, but from the LSP instead labels May 1, 2023
@DaelonSuzuka
Copy link
Collaborator

That would break the compatibility with Godot 3, which is an LTS version of Godot as of now. Ideally, automatic LSP port discovery should be a thing, with "6008" and "6005" being default options.

This is exactly correct, breaking the existing behavior is not acceptable. I've already been working on this feature, but it's lower priority than finishing the big debugger PR.

@jpcerrone
Copy link
Author

jpcerrone commented May 1, 2023

Also fixes a potential bug in the check_client_status() function. It was wrongly getting the serverProperty from the serverPort configuration and viceversa.

Got it. I'm closing this PR and opening #472 to address the check_client_status() fix.

@jpcerrone jpcerrone closed this May 1, 2023
@jpcerrone jpcerrone deleted the changeDefaultLSPPorts branch May 1, 2023 18:47
@raylu
Copy link

raylu commented May 1, 2023

perhaps we could check the version by running ${editor_path} --version?

alternatively, a fallback port and have the extension try both ports in sequence?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived bug language server Issue is not originating from this extension, but from the LSP instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants