search

minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.26k stars 106 forks source link

Guidance on consult-lsp #263

Closed gagbo closed 3 years ago

gagbo commented 3 years ago

Hello,

Sorry if it’s not the correct channel to talk about this, I’m not sure what the best way is.

I’m currently having a go at mimicking a few features of helm-lsp using consult, and I have a few questions regarding usage of consult because the documentation is mode end-user-oriented and less developer-oriented (not a reproach):

I know my version is very crude for now, it’s not pretty and just goes to the candidate on selection, but it currently does what I missed from lsp-mode API, having all the symbols in workspace and having all diagnostics. So maybe it’s a good state to talk about code style and possible ameliorations.

PS: I know I forgot the autoload on consult-lsp-symbols, I’ll add it soon-ish

minad commented 3 years ago

Hi @gagbo!

Thank you for working on this! This is the right channel to ask questions.

Is it okay to depend a lot on consult-- as I do now ? I fear that most of the API I use now might change and trigger a huge refactoring. I’m not sure if I’m supposed to rely that much on it, or if it’s the "expected" usage rate.

Yes, it is okay to use internal functions. The Consult API is also end-user oriented for now, but recently the package has stabilized, so not much breakage is to be expected. See https://github.com/melpa/melpa/pull/7477#issuecomment-803768878 for a more detailed rationale regarding the current Consult API.

When I hit ESC to cancel a completion (I use selectrum as well), state seems to still be called with nil as argument, how can I avoid this ?

There is no way to avoid this since the state function protocol is defined like this. If you abort the completion with C-g or ESC, the state function is called with nil argument in order to do cleanup.

lsp-mode has a callback system for its asynchronous requests. Is there a constructor that I missed to build an "async" source from a (lambda (async &rest args)) just like the consult--async-command ? It’d be nice to have a convenience macro for that as well for async elisp lambdas.

No such macro exists since the only async sources have been external processes. There are also the consult-notmuch, consult-spotify and consult-recoll packages which may or may not use different async sources, e.g., query the web. I am wary of adding convenience macros or other helpers which will only be used by other packages, since there is a risk that they will get outdated. If it turns out that we have to replicate a lot of code in consult-* extension packages, we can still reconsider and move functionality into consult. You may have seen https://jao.io/blog/2021-01-21-consulting-spotify-in-a-better-way.html? I experimented with async websearch at some point, see ttps://github.com/minad/consult/commit/7542ae1ac842c3023759de877ec1e5be29972072, but this is probably severely outdated by now.

I know my version is very crude for now, it’s not pretty and just goes to the candidate on selection, but it currently does what I missed from lsp-mode API, having all the symbols in workspace and having all diagnostics. So maybe it’s a good state to talk about code style and possible ameliorations.

It looks already pretty good. You may also want to talk to the lsp authors or the authors of helm-lsp/ivy-lsp for comments. I think @leungbk is involved with lsp and he tried consult at some point and maybe still uses it?

Regarding packaging this up - my general feeling is that it should go to an extra repository instead of having it maintained here. The base functionality provided by this repository is relatively stable and not under heavy development (I am only depending on Emacs core and then there is flycheck). In contrast, when I update my Emacs I basically see an LSP update every day! Furthermore it is also a bit about distributing the maintenance load. What do you think?

gagbo commented 3 years ago

There is no way to avoid this since the state function protocol is defined like this. If you abort the completion with C-g or ESC, the state function is called with nil argument in order to do cleanup.

Oh okay so I just need to check for cand in my state function, I’ll do that.

I am wary of adding convenience macros or other helpers which will only be used by other packages, since there is a risk that they will get outdated.

Sure, I was a little worried because I thought that it was better to have a single consult--async-form macro to call in my file, instead of replicating the consult--async-* pipes call chain. But if that API is stable that’s fine :)

my general feeling is that it should go to an extra repository instead of having it maintained here

Yeah I agree, if anything it should be packaged with lsp-mode to follow the LSP spec updates (the accessor functions are actually generated from a json schema). But having a separate repo anyway is the correct thing to do I think, I intended to keep it that way.

I’ve asked @yyoncho help to kickstart, but mostly I think the 2 main features we’d want are those. Most of the other magic would be to add to an embark package (that I don’t think I’ll make because I don’t expect to need them), to apply code-actions on completion candidates without needing to jump to a location or things like that.

minad commented 3 years ago

Sure, I was a little worried because I thought that it was better to have a single consult--async-form macro to call in my file, instead of replicating the consult--async-* pipes call chain. But if that API is stable that’s fine :)

Ah okay I see. Yes, the API is designed such that you could smash everything together into a single closure. But it is obviously much better to use the already prepared consult--async-* functions. I made them to be reused :)

Yeah I agree, if anything it should be packaged with lsp-mode to follow the LSP spec updates (the accessor functions are actually generated from a json schema). But having a separate repo anyway is the correct thing to do I think, I intended to keep it that way.

Great, that is good to hear!

I’ve asked @yyoncho help to kickstart, but mostly I think the 2 main features we’d want are those. Most of the other magic would be to add to an embark package (that I don’t think I’ll make because I don’t expect to need them), to apply code-actions on completion candidates without needing to jump to a location or things like that.

Sounds good. Embark actions are nice to have but they can always be added afterwards if needed. Note that Embark keymaps are just normal keymaps binding normal commands, so users can even configure these maps themselves. Or they can just call the actions via M-x embark-act RET M-x <action> manually on the candidate. The only thing you should do is to specify a completion category via the :category argument of consult--read, such that Embark can find the correct keymap.

minad commented 3 years ago

@gagbo I close this since your project seems to be on a good way. Please let me know if further questions come up. I would appreciate it if you ping me as soon as your package hits MELPA, then I can add a link to the readme.