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

ctx context.Context is not propagated anywhere #27

Open
nfx opened this issue Mar 19, 2024 · 4 comments
Open

ctx context.Context is not propagated anywhere #27

nfx opened this issue Mar 19, 2024 · 4 comments

Comments

@nfx
Copy link
Contributor

nfx commented Mar 19, 2024

i expected callbacks to have this shape:

TextDocumentCodeAction: func(ctx context.Context, context *glsp.Context, params *protocol.CodeActionParams) (any, error) {
...

or even just

TextDocumentCodeAction: func(ctx context.Context, params *protocol.CodeActionParams) (any, error) {
   gctx := glsp.FromContext(ctx)
   ...

or even just (you can generate concrete types with go generate ./... and not have loosely-typed lspctx.Notify(protocol.ServerTextDocumentPublishDiagnostics, &protocol.PublishDiagnosticsParams{

TextDocumentCodeAction: func(ctx context.Context, params *protocol.CodeActionParams) (any, error) {
   /// ..
   protocol.NotifyTextDocumentPublishDiagnostics(ctx, &protocol.PublishDiagnosticsParams{
	URI:         uri,
	Diagnostics: res.Diagnostics,
   })

....

but at the moment it's quite difficult to work with this library

@nfx
Copy link
Contributor Author

nfx commented Mar 19, 2024

you can parse the LSP spec with some markdown & https://github.com/dop251/goja and have things pretty code-generatable.

e.g. instead of TextDocumentCodeAction: func(ctx context.Context, params *protocol.CodeActionParams) (any, error) {, you can have clients to write func (x *myImpl) TextDocumentCodeAction(ctx context.Context, params *protocol.CodeActionParams) (protocol.CodeActionResponse, error) { if you have interfaces:

type TextDocumentCodeActions interface {
   TextDocumentCodeAction(context.Context, *CodeActionParams) (CodeActionResponse, error)
}

@tliron
Copy link
Owner

tliron commented Mar 19, 2024

As for the first comment, that is a good idea. I think rather than change the entire API, I will add the Go context into the GLSP context as a field. What do you think? I'll submit a PR and let you review.

As for the second comment, it doesn't seem related to the first. :) Yes, generating much of this library automatically was considered, whether with goja or really anything else (a Go code generator doesn't actually have to be written in Go, you know). But I found that it would be quicker for me to do things manually, also as an exercise of reading the entire LSP spec and understanding it. The initial version of this library took just 2 days of work! If you have an idea of redoing this library via code generation, would be happy to see your implementation of such a generator.

@nfx
Copy link
Contributor Author

nfx commented Mar 19, 2024

it's just the convention for the Go ecosystem nowadays to have ctx context.Context as the first parameter of everything involved with IO 🤷‍♂️

tliron added a commit that referenced this issue Mar 19, 2024
See #27

Also various cleanups and improvements to example server
@tliron
Copy link
Owner

tliron commented Mar 19, 2024

it's just the convention for the Go ecosystem nowadays to have ctx context.Context as the first parameter of everything involved with IO 🤷‍♂️

Sure, but it's also bad practice to inject arbitrary user data into the Go context, which ostensibly is designed for IO cancellation. The user data feature was included in Go context to provide extra user data related to cancellation. Of course it's abused quite a lot. :)

I also think it's cumbersome to add more than one context to every function call. So, my humble design decision is to include an optional Go context into GLSP context, which is already provided.

I just merged the fix, I think it's very straightforward. Could you please take a look and test?

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

No branches or pull requests

2 participants