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

Replace sourcegraph/go-lsp with gopls' internal/lsp/protocol #311

Merged
merged 3 commits into from
Nov 20, 2020

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Nov 19, 2020

Closes #117

The most visible difference between the two libraries is probably the change from int to float which may look awkward especially in the context of line or column numbers, but this is mostly a side effect of the code being generated from TypeScript, where number may or may not have a fractional part. TypeScript does not differentiate so strictly between float and int unlike Go.

@radeksimko radeksimko added the enhancement New feature or request label Nov 19, 2020
@radeksimko radeksimko changed the base branch from master to main November 20, 2020 09:11
// when len(cc.Hover.ContentFormat) == 0, but that's not possible
// without changing lsp.Hover.Content field type to interface{}
//
// We choose to follow gopls' approach (i.e. cut off old clients).
Copy link
Member Author

Choose a reason for hiding this comment

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


func decodeRequestID(v interface{}) (string, error) {
if val, ok := v.(string); ok {
return val, nil
Copy link
Member Author

Choose a reason for hiding this comment

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

sourcegraph/go-lsp actually quoted string IDs

https://github.com/sourcegraph/go-lsp/blob/219e11d77f5d414b4fe5ba7e532b277fca34c1b4/jsonrpc2.go#L25

but I'm considering that a bug which we never encountered because of how rarely strings are used by language clients (most use numbers, which in case of go-lsp were interpreted as uint64 and in gopls' as float64). Either way, if the client expects the server to quote the ID I would consider that a bug on the client side too, because from what I can tell the spec doesn't mention anything about quoting IDs:
https://microsoft.github.io/language-server-protocol/specifications/specification-current/#requestMessage


url := fmt.Sprintf(urlFmt, goplsRef)

resp, err := http.Get(url)
Copy link
Member Author

@radeksimko radeksimko Nov 20, 2020

Choose a reason for hiding this comment

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

This is a simpler way of extracting it compared to govim's approach, which we can apply just because we're only interested in the protocol structs and nothing else and thankfully everything is contained within that one file and that file does not have any non-stdlib imports.

It may need to be revisited if/when we consider extracting anything else.

@radeksimko radeksimko marked this pull request as ready for review November 20, 2020 12:58
@radeksimko radeksimko closed this Nov 20, 2020
@radeksimko radeksimko reopened this Nov 20, 2020
@radeksimko
Copy link
Member Author

CI tests will get triggered once #313 is merged and this PR is rebased.

@radeksimko radeksimko requested a review from a team November 20, 2020 13:38
It would be best to import this like a normal package, but
gopls team doesn't feel confident about the stability of their
generator and so this remains under internal for now.

That said this is still currently the most accurate and up to date
structs we can get for Go today with little effort.
langserver/handlers/handlers_test.go Outdated Show resolved Hide resolved
@radeksimko radeksimko merged commit a9333fd into main Nov 20, 2020
@radeksimko radeksimko deleted the gopls-lsp-structs branch November 20, 2020 19:13
@ghost
Copy link

ghost commented Dec 20, 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 Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create/Use an alternative library for LSP structs
2 participants