fix: Deprecated events-buffer-scrollback-size

[AlexanderArvidsson]

Deprecation was introduced in jsonrpc 1.0.22 in this commit.

This PR ~bumps required version to 1.0.22 and~ now uses recommended :events-buffer-config. It's backwards compatible by checking for existence of -events-buffer-config slot on jsonrpc-process-connection.

[AlexanderArvidsson]

~Unsure if the change was introduced in 1.0.23 or in 1.0.22. The change is there in 1.0.22 on git, but that doesn't necessarily mean that release had the change in it.~

~To be safe, perhaps set required 1.0.23 instead?~

Edit: Double checked in the 1.0.22 tar file, it doesn't have the change. Changed to 1.0.23 as required package.

[ethan-leba]

We should probably maintain backwards compat since jsonrpc is built-in?

[AlexanderArvidsson]

@ethan-leba I actually tried that, but it ended up becoming super difficult. My first thought was to detect if :events-buffer-config was available, but this was not possible without doing some pretty advanced eieio stuff. My second thought is to try and detect which version of jsonrpc is installed, and conditionally change it. Third thought is to check for existence of a function or variable that was introduced in the same commit. But this is not future proof, since the condition won't directly depend on the existence of :events-buffer-config.

However, even if we had a working condition, conditionally choosing the keyword to use is really beyond my elisp knowledge. We could duplicate the entire make-instance call, and conditionally do it based on some condition (either with the complex eieio or jsonrpc version check).

If I had to do backwards compatibility, I would try to get the jsonrpc version, then duplicate the make-instance call to do different things. I'm willing to attempt this, unless you have a better idea how to conditionally apply different keywords based on some condition. I'm not sure how to get the jsonrpc version, but I'll see if I can figure something out.

Finally, perhaps a hot take, but in my opinion, people can pin their version if they want backwards compatibility.

[AlexanderArvidsson]

So it is possible to get the jsonrpc version via (package-version-join (package-desc-version (car-safe (cdr (assq 'jsonrpc package-archive-contents))))), however, this will error if package--initialized is nil, and initializing package.el takes quite a bit of time.

As such, I'm not comfortable relying on package.el to give me the jsonrpc version because it would slow down opening the first file after starting emacs. Either there's a different way, or a different condition needs to be chosen.

Do you have any idea how to check if the events-buffer-config slot is present on jsonrpc-process-connection?

[AlexanderArvidsson]

@ethan-leba Pushed changes for backwards compatibility based on inspecting the slots via eieio-class-slots. Please take a look. This stuff is kind of new to me, so if you have a better way of checking for existence for events-buffer-config I'm happy to change it!

I haven't verified that this works on emacs installations that don't have jsonrpc>=1.0.23 since I don't have time right now to create such an environment. But, I did check if my copilot--slot-exists-p method returns t and nil for -events-buffer-config and a non-slot respectively, so I'm pretty confident it should work.

When it comes to eieio dependency, do we need to add that to the Package-Requires section?

[ethan-leba]

I think I was mistaken actually -- jsonrpc appears to be an ELPA package, not builtin so I think the original solution should work (https://github.com/zerolfx/copilot.el/pull/224/commits/d47c7c603c1dd24d28a33a933df93764f3d458d5)? https://elpa.gnu.org/packages/jsonrpc.html Sorry for the confusion!

Anyways, here's a good way to get the slots on a class:

(thread-last
  (find-class 'jsonrpc-process-connection)
  (eieio-class-slots)
  (mapcar (lambda (s) (cl--slot-descriptor-name s))))

[AlexanderArvidsson]

@ethan-leba Actually, jsonrpc can be builtin. Check list-package, it shows entries for both ELPA and built-in:

  jsonrpc                        1.0.23         available    elpa     JSON-RPC library
  jsonrpc                        1.0.23.0.20231227.75345 available    gnu-devel JSON-RPC library
  jsonrpc                        1.0.16         built-in              JSON-RPC library

If I recall, jsonrpc is added in certain distributions of emacs, and I think both my Arch and Mac has it builtin.

[ethan-leba]

Oh interesting - regardless, I think the original solution should still be fine, the main concern would be if jsonrpc was tied to the version of emacs which it's not since the ELPA version is available as well (which is about the only type of backwards compatibility anyone cares about in Emacs, really..)

[AlexanderArvidsson]

So, do you prefer to revert the backwards compatibility I added (I could change the slot checking to your code, since it's much cleaner), or keep the backwards compatibility, since I already wrote it?

[ethan-leba]

IMO, I think https://github.com/zerolfx/copilot.el/pull/224/commits/d47c7c603c1dd24d28a33a933df93764f3d458d5 would be best in the name of keeping code simple -- I'll defer to @zerolfx though.

[AlexanderArvidsson]

@zerolfx is a retired maintainer, so we'd probably want @emil-vdw's or @rakotomandimby's opinion. In the meantime, I've simplified the slot checking to the following:

(defun copilot--slot-exists-p (slot-list slot-name)
  "Return t if SLOT-NAME exists in SLOT-LIST, nil otherwise."
  (cl-some (lambda (slot) (eq slot-name (cl--slot-descriptor-name slot))) slot-list))

[emil-vdw]

IMO, I think d47c7c6 would be best in the name of keeping code simple -- I'll defer to @zerolfx though.

I also do prefer the simplicity of making this (backwards incompatible) change. I don't see any major reason to maintain backwards compatibility. Even for built in versions of jsonrpc < 1.0.23 a newer version is available from the gnu archive right?

In any event, jsonrpc version 1.0.23 provides backwards compatibility for the old keyword in this release even if a deprecation warning is displayed.

[AlexanderArvidsson]

@emil-vdw @ethan-leba I reverted back to d47c7c6, keeping the history. If you ever decide you want backwards compatibility, you can pick it up from d00ba0d.

Feel free to merge.

[xmatos-so]

Hello! Thank you so much for your work on this emacs package! The jsonrpc upgrade is incompatible with the stock eglot available with emacs 29.1. I understand the reasoning behind keeping the code simple, but I think you should use the standard jsonrpc package. What I'm doing right now is, I fetched AlexanderArvidsson remote repo, merged the latest from main and reverted 2395161.

[AlexanderArvidsson]

@xmatos-so Personally, I agree with you, and I don't think that the backward compatibility I had made the code too complex. Since eglot is incompatible, @emil-vdw perhaps it's better to go with the backward compatibility?