-
Notifications
You must be signed in to change notification settings - Fork 158
feat: customizable lsp event handler and lsp server log #339
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
Conversation
e40947d
to
b5b64d8
Compare
b8bec3d
to
fda007f
Compare
@knilink Lots of changes happened since you've opened this PR, but I think your idea is a good one. Are you still interested in pursuing it? |
fda007f
to
bd3f509
Compare
conflict resolved, looks like this line is the only one causing the conflict Line 724 in 5fa27b5
also renamed the log buffer from |
'window/logMessage | ||
(lambda (msg) | ||
(copilot--dbind (:type log-level :message log-msg) msg | ||
(with-current-buffer (get-buffer-create "*copilot-language-server-log*") |
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 think it's a good idea to extra the buffer names to constants.
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.
ATM, I was trying to stay consistent with the existing code and I think this can be addressed in a different PR.
There were inconsistencies as well such *copilot stderr*
doesn't use hyphen like the rest which can be also fixed together.
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.
Yeah, that's a fair point.
copilot.el
Outdated
"Hash table storing lists of notification handlers.") | ||
|
||
(defun copilot-on-notification (method handler) | ||
"Register HANDLER to be called when a notification of type METHOD is received." |
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.
Is method really the right term here? Perhaps notification-type
or something like this?
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.
rephrased to follow the description in vscode language server doc
@@ -311,6 +311,27 @@ For example: | |||
(setq copilot-network-proxy '(:host "127.0.0.1" :port 7890)) | |||
``` | |||
|
|||
### copilot-on-request |
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.
Please, add blank lines after each heading.
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.
fixed
Register a handler to be called when a request of type method is received. Return JSON serializable as result or calling `jsonrpc-error` for errors. [readmore](https://www.gnu.org/software/emacs/manual/html_node/elisp/JSONRPC-Overview.html) | ||
|
||
For example: | ||
```elisp |
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.
And before the code snippets.
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.
fixed
bd3f509
to
66f7551
Compare
Great work! Thanks! I'm looking forward to #343. 😉 |
copilot-on-notification
,copilot-on-request
window/logMessage
handler and buffer*copilot-language-server-log*
for lsp server logging