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

Explicit support for serverInfo #274

Closed
nemethf opened this issue Oct 18, 2022 · 3 comments · Fixed by #276
Closed

Explicit support for serverInfo #274

nemethf opened this issue Oct 18, 2022 · 3 comments · Fixed by #276

Comments

@nemethf
Copy link
Contributor

nemethf commented Oct 18, 2022

If an editor connects to an already started server using TCP, then it's extremely beneficial to know the server's serverInfo. Unfortunately, not many pygls-based servers send this info. This could be mitigated if instead of:

server = LanguageServer()

the server could be instantiated as

server = LanguageServer(name="my-lsp-server", version="0.1")

Since #273 talks about breaking changes, mandating these arguments would be even better:

server = LanguageServer("my-lsp-server", "0.1")

I can try to write a PR if you think this is acceptable. Thank you.

@tombh
Copy link
Collaborator

tombh commented Oct 18, 2022

Good point about taking advantage of the breaking change! I added a not about it on #273 actually, to see whether anybody else has any ideas to include in the release.

So about this issue. I think it's a good idea. Can you give an example of a specific case where this would make a difference? Is there anywhere in the LSP spec itself that mentions requiring these, or at least suggesting how important they are?

I wonder how possible it is to automatically derive those values from say setup.cfg, folder names, __version__ definitions etc?

But generally I think it would be a good idea to require those fields in the instantiation.

@nemethf
Copy link
Contributor Author

nemethf commented Oct 18, 2022

Can you give an example of a specific case where this would make a difference? Is there anywhere in the LSP spec itself that mentions requiring these, or at least suggesting how important they are?

The specification doesn't require these because of backward compatibility. But I think it's OK for pygls to be a bit more restrictive than the specification in this case.

I read several issues reported to a client where the reporters were not sure about their systems, for example, they wanted to start pyright and but actually started pylsp. Or what is more relevant to pygls, they failed to identify the version of their server. If the server sends this information, it will appear in the communication logs, which the reporters need to attach to the issues.

I wonder how possible it is to automatically derive those values from say setup.cfg, folder names, __version__ definitions etc?

If automation is possible, that's even better. As far as I know pygls cannot be sure any of the methods you listed would work for a pygls-based server. But I'd like to be proven wrong.

@tombh
Copy link
Collaborator

tombh commented Oct 18, 2022

The specification doesn't require these because of backward compatibility. But I think it's OK for pygls to be a bit more restrictive than the specification in this case.

I agree. I think Pygls has an interesting role to play. I don't think its responsibility is merely to act as a 100% faithful reflection of the LSP spec. LSP already is the single source of canonical truth. Pygls therefore can take more of a role in demonstrating sane real world usage. An "ambassador" for LSP.

I read several issues reported to a client where the reporters were not sure about their systems, for example, they wanted to start pyright and but actually started pylsp. Or what is more relevant to pygls, they failed to identify the version of their server. If the server sends this information, it will appear in the communication logs, which the reporters need to attach to the issues.

I don't completely understand those points, but nevertheless they make me think of 2 very valid points. Both of which relate to encouraging best practices. 1. A language server should always broadcast a unique name to clients. A user inside their editor should be able to easily identify a server in order to say, restart it or filter its logs. 2. Language server developers would benefit from being able to easily and consistently know the version of the server that a user is reporting an issue for.

So anyway, basically I'm very convinced by your suggestion and think it would be a great addition to Pygls, so definitely worth a PR to get feedback from other LSP project developers.

I'll have a think about the automation of a name and version. I think you're right that there's no way to consistently guarantee the locations of those variables.

nemethf added a commit to nemethf/pygls that referenced this issue Oct 21, 2022
These are sent as serverInfo in InitializeResult

Clsoe openlawlibrary#274
nemethf added a commit to nemethf/pygls that referenced this issue Oct 21, 2022
These are sent as serverInfo in InitializeResult

Close openlawlibrary#274
nemethf added a commit to nemethf/pygls that referenced this issue Oct 21, 2022
These are sent as serverInfo in InitializeResult

Close openlawlibrary#274
nemethf added a commit to nemethf/pygls that referenced this issue Oct 29, 2022
These are sent as serverInfo in InitializeResult

Close openlawlibrary#274
tombh pushed a commit to nemethf/pygls that referenced this issue Oct 29, 2022
These are sent as serverInfo in InitializeResult

Close openlawlibrary#274
tombh pushed a commit that referenced this issue Oct 29, 2022
These are sent as serverInfo in InitializeResult

Close #274
tombh added a commit that referenced this issue Nov 2, 2022
Added:
- Add `name` and `version` arguments to the constructor of `LanguageServer` ([#274])
[#274]: #274
Changed:
- Default behaviour change: uncaught errors are now sent as `showMessage` errors to client.
  Overrideable in `LanguageServer.report_server_error()`: #282
Fixed:
- `_data_recevied()` JSONRPC message parsing errors now caught
@tombh tombh mentioned this issue Nov 2, 2022
tombh added a commit that referenced this issue Nov 2, 2022
Added:
- Add `name` and `version` arguments to the constructor of `LanguageServer` ([#274])
[#274]: #274
Changed:
- Default behaviour change: uncaught errors are now sent as `showMessage` errors to client.
  Overrideable in `LanguageServer.report_server_error()`: #282
Fixed:
- `_data_recevied()` JSONRPC message parsing errors now caught
tombh added a commit that referenced this issue Nov 2, 2022
Added:
- Add `name` and `version` arguments to the constructor of `LanguageServer` ([#274])
[#274]: #274
Changed:
- Default behaviour change: uncaught errors are now sent as `showMessage` errors to client.
  Overrideable in `LanguageServer.report_server_error()`: #282
Fixed:
- `_data_recevied()` JSONRPC message parsing errors now caught
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 a pull request may close this issue.

2 participants