Split `eglot-code-actions` into smaller functions

[caioaao]

Problem description

Currently there's no way of changing completing-read for something else (eg.: helm) for listing/selecting an action from eglot-code-actions.

Proposed solution

This PR tries to solve this limitation by splitting the functionality and enabling one to use its parts. It basically divides the current implementation into a function to fetch the actions available, a function to execute an action chosen by the user, and a function to tie these two and present it using completing-read - preserving the current behavior.

An example of building upon these parts is presented in this gist. In it, I implemented the same functionality but using helm to present the actions, as it is my preferred tool. https://gist.github.com/caioaao/0fdfa0fc2d86ef1fbdfc39f509d62319

Alternatives considered

• Add a custom variable so the user can pick which program to use for showing the code actions. While this might be easier for people with less experience in elisp, it'd increase complexity of Eglot's code and put the burden of supporting different customizations on to Eglot project.

[caioaao]

@joaotavora sorry about that. I thought creating a gist with my use-case would be clearer and faster for understanding the PR. I edited the description with a more extensive explanation to this refactor

[joaotavora]

sorry about that

Don't worry, and I'm sorry myself for my knee-jerk reaction. I should have thanked you for your interest and contribution first :-) I'll have a good look when I find the time.

[skangas]

I'm not sure I understand the use case. I'm not saying it's invalid or anything, just that I would like some additional details to better understand it.

From a quick overview, it seems like this will allow you to extend the helm completion framework to work together with eglot-code-actions, which is currently not possible? Or am I misunderstanding things completely?

[caioaao]

That's right. Right now (or at least at the time of this PR) we can't use anything other than completing-read. Splitting the functions allows us to build custom integrations like using helm or ivy

[joaotavora]

But @caioaao, completing-read is the meeting point between Emacs completion frontends and backends (along with completion-at-point-functions and some others of course), so I must be missing something. Creating new API points in Eglot for Helm etc to use would just sidestep those existing interfaces (and create more integration work for the future). I'm not sure we want that.

[joaotavora]

Also, I'm reasonably sure (but not 100%) that Helm, Ivy, vertico, and probably more can work with completing-read. Have you checked their docs?

[caioaao]

It’s been a while since I looked into this, but afaik the only solution to make it work with the current implementation of Eglot was to reimplement the function and call it instead. Since we’re talking about a different front end it makes sense not to use completing-read, no?

On Tue, 11 Jan 2022 at 17:43 João Távora @.***> wrote:

Also, I'm reasonably sure (but not 100%) that Helm, Ivy, vertico, and probably more can work with completing-read. Have you checked their docs?

— Reply to this email directly, view it on GitHub https://github.com/joaotavora/eglot/pull/622#issuecomment-1010344565, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKU2FQ73TFTNPZHJXBTEWTUVSJABANCNFSM4XYKPV5Q . You are receiving this because you were mentioned.Message ID: @.***>

[nemethf]

I haven't followed this discussion, but https://github.com/emacs-helm/helm/wiki says "helm-mode enables Helm completion globally for any Emacs command using completing-read or read-file-name." So, if something wrong with integrating Helm with Eglot, it's most probably elsewhere.

[caioaao]

Ahh, ok. I see where the confusion lies now. helm-mode is a complement to helm: You can use helm's capabilities without using helm-mode and helm-mode basically monkey patches Emacs' completing-read and will replace its usage in anything that calls that function.

While I agree that helm-mode could be used to integrate helm with eglot, I'd say it wouldn't hurt having eglot's code a bit more flexible so we can extend its functionalities without having to resort to overriding core emacs functions. This API also makes it clear that the new functions are internal so they may change in the future and you don't have to worry that much about maintaining backwards compatibility (even though I can't see why you wouldn't). Also, smaller and more composable functions never hurt anyone, right? :)

[joaotavora]

@caioaao helm-mode comes with Helm. A complement or not, it's the thing that solves your problem, so there should be no code in Eglot to appease a particular frontend.

Afaik helm-mode does use Emacs standard interfaces. https://github.com/emacs-helm/helm/blob/72c61b2d0cb3cd48fb1b24d7708ad1794eeeb10c/helm-mode.el#L62 So

[caioaao]

@joaotavora I agree and in the end it comes down to personal preference. But regardless, the use case for this PR exists, it doesn't mean it's appeasing a particular frontend, and splitting a function into smaller ones should come with nearly zero downsides, so I'm failing to see what's the hangup

[joaotavora]

it doesn't mean it's appeasing a particular frontend,

you argued earlier about helm support. At a minimum it seems to be appeasing the set of frontends who don't abide by completing-read-function, a cohort that seems inexistent and where I'd rather see that deficiency fixed.

and splitting a function into smaller ones should come with nearly zero downsides,

Depends if you're going to advertise the functions, of course. If you are, there's a maintenance cost from supporting those in the future, a cost I'd rather not incur in. If you decide not to advertise them, then splitting for split's sake also doesn't much sense to me.

I'll let @skangas decide.

[skangas]

Depends if you're going to advertise the functions, of course. If you are, there's a maintenance cost from supporting those in the future, a cost I'd rather not incur in. If you decide not to advertise them, then splitting for split's sake also doesn't much sense to me.

I'll let @skangas decide.

Hmm. I don't hate this change. I can see how it makes the code easier to read. IMO, such refactorings are sometimes worth doing, though I generally tend to do them only when I have a clear reason, for example when I'm adding a test for the now separate function.

However, there is always a cost associated with adding a new function in ELisp. If we don't want this to be used by others, we could perhaps just put the factored out functions in cl-labels instead. Would that make sense?

[joaotavora]

@skangas just a bit of background. There has been at least one case where Doom Emacs was relying on an internal implementation detail (a -- function) and that led to noise in this bugtracker. So even though the -- should be sufficient to explain that it shouldn't be used by other packages, it's clearly not. So there is a risk even in augmenting the number of definitions with the -- thing. The cl-labels idea looks OK. But judging from the initial posts, it could be that the main motivation for this was to have some functions to hook onto from the outside.

[skangas]

@skangas just a bit of background.

Thanks, this helps.

So even though the -- should be sufficient to explain that it shouldn't be used by other packages, it's clearly not. So there is a risk even in augmenting the number of definitions with the -- thing.

Indeed. This was what I was alluding to when I said that "there is always a cost associated with adding a new function in ELisp", although I didn't spell it out explicitly.

The cl-labels idea looks OK. But judging from the initial posts, it could be that the main motivation for this was to have some functions to hook onto from the outside.

Let's see what @caioaao has to say about this.