search

copilot-emacs / copilot.el

An unofficial Copilot plugin for Emacs.
MIT License
1.79k stars 126 forks source link

fix: Better compatible with jsonrpc #259

Closed jcs090218 closed 7 months ago

jcs090218 commented 7 months ago

For #258.

emil-vdw commented 7 months ago

Have you tested on old and new version of jsonrpc?

jcs090218 commented 7 months ago

Yes, it both works for me. :)

emil-vdw commented 7 months ago

Don't you think that it would be better for this "old" jsonrpc version to be hardcoded? It's not really like it can change and we should only use the old parameter when it is older than that specific version not when it is older than the version we currently rely upon. I.e. at some point we may update the version of jsonrpc we depend on but the logic would break.

jcs090218 commented 7 months ago

I'm not sure what you mean. Can you elaborate? 🤔

emil-vdw commented 7 months ago

Nevermind, I was confused there for a second. LGTM. :+1:

jkl1337 commented 7 months ago

This is completely broken. Package-Requires is not at a sufficient version (28.1). Compilation breaks due to missing (require 'package) -- (suggest using package-lint to prevent these issues). Furthermore, package-get-descriptor is only going to work if the descriptor is loaded first, and just requiring 'jsonrpc doesn't do that. As it is this breaks emacs startup.

I haven't dove into this issue, but this approach overall seems suspect. Can this not be done with feature detection?