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

Feature/lsp copilot #4635

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

kassick
Copy link
Contributor

@kassick kassick commented Dec 5, 2024

This PR provides lsp-copilot, a client for the Copilot Node Server.

@github-actions github-actions bot added documentation client One or more of lsp-mode language clients labels Dec 5, 2024
@kassick kassick force-pushed the feature/lsp-copilot branch from e6e694c to a60adf3 Compare December 5, 2024 19:57
@kassick
Copy link
Contributor Author

kassick commented Dec 5, 2024

@kiennq here's the lsp-copilot client.

I've included in the PR a change that i've submitted previously that improves an issue that happens with ruff (probably with others as well) -- lsp-mode broadcasts executeCommands to all registered servers. Ruff does not like that and returns an error instead of silently ignoring the command. The result is that sometimes, after accepting an inline completion, the user sees an error message.

The first commit in the PR fixes that.

Let me know if you prefer to go another way with this, or keep this change outside of this PR and iterate over #4494

@kassick kassick force-pushed the feature/lsp-copilot branch 2 times, most recently from 85dab1c to 2df4263 Compare December 5, 2024 20:08
@kassick
Copy link
Contributor Author

kassick commented Dec 5, 2024

(CI is failing because the compiler is not understanding the -let bindings unpacking the newly defined interfaces... I'm not sure why, I'll take a look as soon as I've got some time, but if you see something obvious, please let me know!)

@kassick
Copy link
Contributor Author

kassick commented Dec 5, 2024

Regarding documentation: I'm not sure how to place the copilot in the language list, and even if that's where it should live. I've placed it in the Language section for lack of a better, obvious place.

clients/lsp-copilot.el Outdated Show resolved Hide resolved
clients/lsp-copilot.el Outdated Show resolved Hide resolved
clients/lsp-copilot.el Outdated Show resolved Hide resolved
clients/lsp-copilot.el Outdated Show resolved Hide resolved
@kiennq
Copy link
Member

kiennq commented Dec 6, 2024

The first commit in the PR fixes that.

Let me know if you prefer to go another way with this, or keep this change outside of this PR and iterate over #4494

I would prefer to separate them.

@kassick kassick force-pushed the feature/lsp-copilot branch from 2df4263 to 1752d1c Compare December 7, 2024 13:24
@kassick kassick force-pushed the feature/lsp-copilot branch from 2896279 to c529d46 Compare December 7, 2024 13:31
@kassick
Copy link
Contributor Author

kassick commented Dec 7, 2024

@kiennq I've fixed the issues you've pointed out, let me know if you think this is good to go or if it needs more work.

@kiennq
Copy link
Member

kiennq commented Dec 8, 2024

@kiennq I've fixed the issues you've pointed out, let me know if you think this is good to go or if it needs more work.

LGTM.
I wonder why you have to create two lsp clients? Shouldn't the second one (the one uses lsp-package-path) covers both of them? If there's system wide copilot lsp executable it will use it (the :system key), else it will use the :npm key that lsp-mode can install.

Now the lsp-controlled copilot npm install path contains a bin with a
copilot-node-server -- we no longer need to go out of our way to find the
language server js and execute it with node
@kassick
Copy link
Contributor Author

kassick commented Dec 9, 2024

I wonder why you have to create two lsp clients? Shouldn't the second one (the one uses lsp-package-path) covers both of them? If there's system wide copilot lsp executable it will use it (the :system key), else it will use the :npm key that lsp-mode can install.

You're right, it's no longer needed!

The second client should have been explicitly marked with :remote t. When I coded the first version of the client, there was no bin/copilot-node-server in the installation path, so (for the local client) I had to find language-server.js and execute it in node explicitly. That approach would not fly in remote, so I had to assume that the target environment would have a server command somewhere in the PATH.

But I've just checked the current version of the copilot-node-server and it contains bin/copilot-node-server. Now with lsp-package-path and this executable script, we no longer need to worry.

Thanks for pointing that out!

ACTUALLY: The executable script is there, but then it complains that it can't find the tokenizer ... calling node explicitly probably sets the CWD to the dirname of the script, then it can find the payload correctly...

I'll have to ammend the last commit to work around that, maybe keeping the same strategy as was present in c529d46

@kiennq
Copy link
Member

kiennq commented Dec 10, 2024

I've tried your last commit, the (lsp-package-path 'copilot-ls) works fine with the latest copilot-node-server.
There's an issue with plist that the :inlineCompletionProvider is mistakenly treated as nil even though it's null.
Here's my fix kiennq@ffe4564

@kassick
Copy link
Contributor Author

kassick commented Dec 10, 2024

I've tried your last commit, the (lsp-package-path 'copilot-ls) works fine with the latest copilot-node-server. There's an issue with plist that the :inlineCompletionProvider is mistakenly treated as nil even though it's null. Here's my fix kiennq@ffe4564

Weird -- I've tested with your fork and copilot did not complain about not finding the tokenizer model ... Let's hope it does not happen again ;)

@kassick
Copy link
Contributor Author

kassick commented Dec 10, 2024

About the changes in your fork -- I've left a couple of comments there.

Other than that, how do you want to proceed? Should I merge your changes here or do you prefer to merge them later?

@kiennq
Copy link
Member

kiennq commented Dec 10, 2024

Other than that, how do you want to proceed? Should I merge your changes here or do you prefer to merge them later?

Yes, please take my commit and make changes as you see fit.

@kassick
Copy link
Contributor Author

kassick commented Dec 11, 2024

Done @kiennq !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants