Skip to content

Conversation

jmpunkt
Copy link
Contributor

@jmpunkt jmpunkt commented Mar 4, 2025

Feature

This allows users to set the settings (https://github.com/github/copilot-language-server-release?tab=readme-ov-file#configuration-management) for the Copilot LSP. VSCode stores this in settings.json? Having this feature allows users to login into GitHub Enterprise instances. Without this it is not possible. You can also disable telemetry with this.

How To Test

Testing this without a GHE instance is tricky but doable.

  1. Start Emacs and evaluate this PR.
  2. Log out of your current session (M-x copilot-logout).
  3. Use eval-last-sexp (C-x C-e) to step through the following snippets
(copilot--start-agent)
(copilot-login)
;; you should be able to login, everything works fine (abort login C-g)
(setopt
 copilot-lsp-settings
 '(:github-enterprise (:uri "https://example2.ghe.com")))
(copilot-login)
;; there should be an error, the instance does not exist, the error code shows the url

Another test can be:

(setq copilot-log-max 2000)
(copilot--start-agent)
(switch-to-buffer-other-window "*copilot events*")
(setopt
 copilot-lsp-settings
 '(:github-enterprise (:uri "https://example2.ghe.com")))
;; you will see in the log that the telemetry tries to fetch from the instance (with an error of course)
(setopt
 copilot-lsp-settings
 '(:github-enterprise (:uri "https://example2.ghe.com")
   :telemetry (:telemetryLevel "all")))
;; now the previous telemetry error is gone

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 30, 2025

I think you should also mention this in the README.

@jmpunkt jmpunkt force-pushed the add-settings-support branch from 80fd2a5 to 4c58853 Compare April 3, 2025 16:42
@jmpunkt jmpunkt force-pushed the add-settings-support branch from 4c58853 to ac142db Compare April 3, 2025 16:46
@jmpunkt
Copy link
Contributor Author

jmpunkt commented Apr 3, 2025

I added the README entry. If there is something else missing, then let me know.

copilot.el Outdated
@@ -420,6 +443,13 @@ SUCCESS-FN is the CALLBACK."
;; handle older jsonrpc versions
(funcall make-fn :events-buffer-scrollback-size copilot-log-max)))))

(defun copilot--notify-configuration ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "copilot--notify-configuration-changed" is probably a better name for this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function got removed. We only notify the server at the start now.

copilot.el Outdated
Exchange the URI with the correct URI of your organization."
:set #'copilot--lsp-configuration-changed
:type 'sexp
:group 'copilot)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a :package-version property mentioning this got added in version 0.2.

copilot.el Outdated
'(:github-enterprise (:uri \"https://example.ghe.com\"))

Exchange the URI with the correct URI of your organization."
:set #'copilot--lsp-configuration-changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm reasonably certain this will be triggered only when using M-x customize, so you'll probably have to mention it somewhere, so people are not surprised that manual config changes don't trigger it.

copilot.el Outdated
@@ -177,6 +177,29 @@ You may adjust this variable at your own risk."
:group 'copilot
:package-version '(copilot . "0.1"))

(defun copilot--lsp-configuration-changed (symbol value)
"Notify the Copilot LSP configuration has changed and set SYMBOL to VALUE."
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should probably add a note that's supposed to get triggered when copilot-lsp-settings gets updated via the customize UI. Such callbacks are rarely used and might surprise many users.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2025

I've added a few small remarks here and there, but overall your changes are in a good shape.

@jmpunkt
Copy link
Contributor Author

jmpunkt commented Apr 4, 2025

Thanks for the review. I had to change one thing. There is an issue with the notify when changing the settings from something (e.g. '(:github-enterprise (:uri "test.com"))) to nil. The change is not recognized by the LSP. We would have to send '(:github-enterprise (:uri nil)) instead, in order to apply the change. So I restart the agent every time the settings change. Otherwise, we would have to calculate the difference between the old and new value.

@bbatsov bbatsov merged commit bb51738 into copilot-emacs:main Apr 4, 2025
13 checks passed
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2025

Great work! Thanks!

bbatsov pushed a commit to bbatsov/copilot.el that referenced this pull request Apr 5, 2025
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

Successfully merging this pull request may close these issues.

2 participants