Generic Handler and Easier Future Protocol Extensions#43
Conversation
|
@tliron when you have a moment, could you enable actions for the repo? |
There was a problem hiding this comment.
Pull Request Overview
This PR rewrites the server-side to use a generic handler interface, swaps out the old commonlog for Go’s structured slog, and introduces translation helpers for future protocol extensions.
- Migrated all loggers to use
log/slogand removedcommonlog - Updated
Serverto acceptglsp.RPCHandlerand centralized request handling viaHandlerHandle - Added generic translation functions and simplified custom request registration
Reviewed Changes
Copilot reviewed 38 out of 42 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/server.go | Swapped in slog.Logger, changed Handler type |
| server/run-websocket.go | Updated WebSocket serving to use slog, error wrapping |
| server/serve.go | Refactored stream and WS methods to use slog.Logger |
| protocol/translation/handler_translation.go | Introduced generic translation handlers |
| protocol/custom.go | Added global custom-method registry and registration API |
Comments suppressed due to low confidence (3)
server/handler.go:81
- [nitpick] Consider adding a doc comment above
HandlerHandleto explain its purpose, parameters, and the meaning of its return values (validMethod,validParams).
func (s *Server) HandlerHandle(context *glsp.Context) (r any, validMethod bool, validParams bool, err error) {
server/run-websocket.go:11
- [nitpick] The parameter named
addressis clear, but internal log variables are namedlog, which shadows thelogpackage in other contexts. For consistency with other methods taking aloggerargument, consider renaming local variables tologger.
func (s *Server) RunWebSocket(address string) error {
|
I think this is ready as soon as we can get CI turned on for the repo |
Add unit tests for the protocol package covering position/range byte indexing, document change (marshal/unmarshal/validation) and URI parsing. New tests added: - protocol/ranges_test.go - protocol/ts_documentchanges_test.go - protocol/uri_test.go Update CI workflow (.github/workflows/test.yml) to run tests for all packages using `go test -race ./...` so the new tests are executed.
|
I think that for the future support im going to go down this route. As |
|
I'll try and have a look in the coming week. |
|
@tris203 I'm not picky on how it gets implemented, just looking for the more recent protocol changes to land. |
Supersedes #42
Essentially a complete rewrite of the handler side to use a generic handler
I accept this is probably almost unreviewable