-
Notifications
You must be signed in to change notification settings - Fork 566
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
LSP Notification Message Type #749
base: main
Are you sure you want to change the base?
Conversation
dyl10s
commented
Apr 4, 2025
- Added lsp notification message type, as the todo comment stated the difference is if an ID is provided or not.
- A few of the messages that were being handled were notifications, so they are all now parsed/typed correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Co-authored-by: Copilot <[email protected]>
internal/lsp/lsproto/jsonrpc.go
Outdated
var raw struct { | ||
JSONRPC JSONRPCVersion `json:"jsonrpc"` | ||
ID *ID `json:"id"` | ||
ID *ID `json:"id,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, we should be using omitzero
(new in Go 1.24) rather than omitempty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other of these, FWIW, I just commented on the one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and normalized this file to always use omitzero!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/lsp/server.go:193
- [nitpick] For unknown request methods that include an ID, consider returning an error (such as using sendError) rather than solely logging the event, to notify clients of invalid requests.
s.Log("unknown method", req.Method)