Skip to content

Conversation

jcs090218
Copy link
Member

For #258.

@emil-vdw
Copy link
Collaborator

Have you tested on old and new version of jsonrpc?

@jcs090218
Copy link
Member Author

Yes, it both works for me. :)

@emil-vdw
Copy link
Collaborator

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
Copy link
Member Author

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

@emil-vdw
Copy link
Collaborator

Nevermind, I was confused there for a second. LGTM. 👍

@emil-vdw emil-vdw merged commit f8283e4 into copilot-emacs:main Feb 15, 2024
@jcs090218 jcs090218 deleted the fix/jsonrpc branch February 15, 2024 12:13
@jkl1337
Copy link
Contributor

jkl1337 commented Feb 15, 2024

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?

jkl1337 added a commit to jkl1337/copilot.el that referenced this pull request Feb 15, 2024
This reverts commit f8283e4.

Completely ridiculous approach.
jkl1337 added a commit to jkl1337/copilot.el that referenced this pull request Feb 15, 2024
@jkl1337 jkl1337 mentioned this pull request Feb 15, 2024
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.

3 participants