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

Unique command names #279

Merged
merged 10 commits into from
Oct 30, 2020
Merged

Unique command names #279

merged 10 commits into from
Oct 30, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Oct 23, 2020

This PR ensures the uniqueness of the exported commands across different language servers (with a terraform-ls. prefix, this is a common practice) and an additional <commandPrefix>., this latter ensures multiple instances of the language server can run side by side, it will become the clients responsibility to send requests with the correct command name, the commandPrefix is only enabled if specified in the initializationOptions for the server.

"executeCommandProvider": {
    "commands": ["1234.terraform-ls.rootmodules"]
}

Closes #278

@appilon
Copy link
Contributor Author

appilon commented Oct 23, 2020

Paul had suggested the unique ID (maybe even just an incrementing integer) be generated by the client and passed to the server to prefix/suffix, which I'm open to as an alternative, or perhaps we use the pid as the unique ID, the client would have to figure out that mapping though.

@aeschright
Copy link
Contributor

For that second option to work, would the server then just keep track of what uuid was used to call it the first time, or is there some other way we'd register the setting?

@appilon
Copy link
Contributor Author

appilon commented Oct 23, 2020

Correct the value would be passed to the server during initialization, it would have the same effect, just maybe feel a little cleaner without the long root folder URI, the client would likely keep the ID as the key to it's map (which as of writing is the workspace folder URI).

Co-authored-by: Audrey Eschright <[email protected]>
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thank you for working on this and for discovering this issue early before the release! 😅

Besides my comments inline I would have one more question about the format - was there a particular reason behind <static-prefix>.<command>.<instance-specific-prefix>?
I don't have a strong opinion on the order, just genuinely curious if this was motivated by anything on the client side or in the LSP?

langserver/handlers/handlers_test.go Outdated Show resolved Hide resolved
langserver/handlers/initialize.go Show resolved Hide resolved
langserver/handlers/execute_command.go Outdated Show resolved Hide resolved
internal/settings/settings.go Outdated Show resolved Hide resolved
internal/context/context.go Outdated Show resolved Hide resolved
@appilon
Copy link
Contributor Author

appilon commented Oct 28, 2020

Thank you for working on this and for discovering this issue early before the release! 😅

Besides my comments inline I would have one more question about the format - was there a particular reason behind <static-prefix>.<command>.<instance-specific-prefix>?
I don't have a strong opinion on the order, just genuinely curious if this was motivated by anything on the client side or in the LSP?

@radeksimko the static prefix is a common pattern. I should maybe see if I can find some precedent with another popular langserver (I'll try python), but I felt that since the instance specific ID is optional, I felt making it a suffix felt more intuitive.

terraform-ls.rootmodules contrasted with terraform-ls.1234.rootmodules
VS
terraform-ls.rootmodules contrasted with `terraform-ls.rootmodules.1234

I liked that the first two pieces <static-prefix>.<command>. "lined up" and the last part can be dropped.

Similar to your other point though... I also now wonder if your suggestion is more intuitive in the world of namespacing like "please route request to terraform-ls instance 1234. Need to chat about it more..

FINAL DECISION:

going with an optional prefix for multi server uniqueness

1234.terraform-ls.rootmodules

Add explicit test with commandPrefix
Add doc on new setting
docs/SETTINGS.md Outdated Show resolved Hide resolved
docs/SETTINGS.md Outdated Show resolved Hide resolved
langserver/handlers/execute_command.go Outdated Show resolved Hide resolved
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

:shipit:

langserver/handlers/execute_command.go Outdated Show resolved Hide resolved
@appilon appilon merged commit c90be01 into master Oct 30, 2020
@appilon appilon deleted the unique-commands branch October 30, 2020 16:31
@radeksimko radeksimko added this to the v0.9.0 milestone Nov 4, 2020
@ghost
Copy link

ghost commented Nov 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the context necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple languageserver instances will crash on vscode
3 participants