Giter Site home page Giter Site logo

Comments (10)

jvdydev avatar jvdydev commented on August 20, 2024 1

ye, I was originally thinking it's different from the tree-sitter code as the it doesn't require a C compiler (compared to tree-sitter), but obviously LSP stuff is not very "just Emacs" either.

If eglot can't find a server, it will most likely ask you to put in an address + port on where to find the server (it essentially assumes you're running a non-standard configuration).
Could be confusing for people that haven't looked into the whole LSP stuff yet.

I feel like this is a pattern we have to figure out long-term for things that require stuff outside of Emacs (compilers, language servers, command-line utils, ...).
Would be nice to also offer a reasonable way to get Crafted Emacs running on a very locked down system where not much is/can be installed.

from crafted-emacs.

mgmarlow avatar mgmarlow commented on August 20, 2024 1

Happy to help! I've been testing the changes today against Rust & TS, I'll update the docs in that PR later today 👍

from crafted-emacs.

jvdydev avatar jvdydev commented on August 20, 2024

Thank you for the issue.

If we wouldn't rely on eglot-server-programs to link the modes, we could do without explicitly loading eglot (as eglot-ensure is an autoload).
I guess we could provide our own server programs list (which I don't think is a good idea) or just (require 'eglot) explicitly in the config.

from crafted-emacs.

mgmarlow avatar mgmarlow commented on August 20, 2024

If we wouldn't rely on eglot-server-programs to link the modes, we could do without explicitly loading eglot (as eglot-ensure is an autoload).

I guess we could provide our own server programs list (which I don't think is a good idea) or just (require 'eglot) explicitly in the config.

Yeah, I figured as much. I was leaning towards the explicit require approach but wanted to check before submitting a PR since it would ditch the eval-after-load.

I suppose another option could be to make it opt-in with a configuration function similar to the one for tree-sitter, but I personally think there's value in simply requiring it and registering the Eglot-ensure calls.

That said, does Eglot-ensure check for the instance of the LSP server binary before activating? Might lead to a few errors for users who only use LSP for a subset of modes, and don't have all of the prerequisite binaries installed.

from crafted-emacs.

jeffbowman avatar jeffbowman commented on August 20, 2024

If there is no server for eglot to use, you get an error (error "[eglot] -1 : Server died"). You still get the language mode loaded however.

lsp-mode may automatically install a server, there is usually some flag somewhere that lets lsp-mode know it can be installed. Eglot deliberately does not do this. Thus if the user knows they will be editing Rust files (for example) it is incumbent on them to install the requisite tooling to allow the rust server to start when needed and add whatever configuration eglot needs to connect to that server. If we take the same viewpoint (which we have so far) then we defer to the user to know what they are doing and take the appropriate action / provide appropriate tooling / etc.

To comment on the reason for this issue, we have to wait until after eglot is loaded because the eglot-server-programs won't be evaluated until then. However, if we switched to a require guard instead of eval-after-load, then the variable would be defined. I see you point about the chicken-and-egg thing. Another possible solution would be for the user to create their own ide module and put 2 lines in it something like this:

;;; my-ide-config.el 
(require 'eglot)
(require 'crafted-ide-config)

That may be a bit obtuse, and we can (maybe) safely assume they want eglot turned on with all the hooks and things just because the added the module to their configuration. However, it is possible they only want the tree-sitter configuration and never intend to use eglot - which means they would never load eglot and therefore none of the hooks would be set, and they won't get the subsequent "Server died" error every time they visit a language mode buffer.

The end result is, we may need to think a little bit on how we want to approach this for people who want the hooks with eglot-ensure vs those who do not.

Would be nice to also offer a reasonable way to get Crafted Emacs running on a very locked down system where not much is/can be installed.

Yes! However, that largely precludes the use of either tree-sitter or eglot since both require things to be downloaded - language grammars for TS modes and LSP servers for eglot. So, for those systems, including crafted-ide is probably not the answer.

from crafted-emacs.

mgmarlow avatar mgmarlow commented on August 20, 2024

Ah @jeffbowman I think I missed your edits before opening #396. Bringing the discussion back over to this issue.

Anecdotally with that change I had just opened up a markdown buffer and had this pop up:

Error in post-command-hook (#[0 "\303\301!\205�\0r\301q\210\304\305\300\242\306#\210
?\205�\0\307\310\311 \")\207" [(#0) #<buffer list_processing.md> eglot--managed-mode buffer-live-p remove-hook post-command-hook t apply eglot--connect eglot--guess-contact] 4]): (error "None of ’marksman, vscode-markdown-language-server’ are valid executables")

This got me thinking, I personally would not want to install a markdown LSP and it's very annoying to have the error pop up when visiting every markdown file (since Eglot will retry). So maybe this is a mark against my PR and automatically setting this up by default. (Noteworthy that this problem is still present w/o any changes posted, it just requires a user to execute Eglot at some point before the failed server attempts appear.)

With that in mind, I'm wondering a bit more about this feature. Is configuring eglot-ensure for every known Eglot LSP a useful addition, or should the user be wholly responsible? I do think it's convenient for my configured LSPs to automatically enable, but the list that I personally use is down to 3 or 4 languages.

I'm leaning towards one of these three:

  • Don't try to enable eglot-ensure for the user at all.
  • The user opts into eglot-auto-detect-all or similar to configure eglot-ensure for all eglot-server-programs. This works similarly to the tree sitter configuration in that it requires the user to call the function in their config after crafted-ide-config is loaded.
  • crafted-ide-config will configure eglot-ensure out of the box, but only for LSP binaries that exist on the machine (e.g. via executable-find)

from crafted-emacs.

jeffbowman avatar jeffbowman commented on August 20, 2024

Is configuring eglot-ensure for every known Eglot LSP a useful addition, or should the user be wholly responsible?

Yep, this is the "big money" question. The "simple" (aka naive) options are:

  • load them all
  • load none of them

We opted for the former especially since the tree-sitter configuration takes the same approach. That side is a bit more reasonable because nothing happens if a language mode is not used, or if the grammar for that language is not present, the TS language mode is also likely missing, so no errors are likely.

... list that I personally use is down to 3 or 4 languages.

Yep, same. And I expect this is likely true for the majority of users. I don't think it's very likely someone is programming in all the languages for which there is an LSP available.

A couple of options I can think of quickly:

  • Provide a configuration option to allow the user to supply a list of LSP modes to enable, then use either a defcustom initializer to add the hooks for those or a defun the user would add to their configuration which reads the value of the custom option and adds the hooks. I like the former better.
  • Provide a defun which accepts a list of modes for which to enable eglot.

Advantages of the former which makes it my preference:

  • The option is available through the Easy Customization facilities, which allows the user to customize the option without having to touch lisp.
  • The value is saved in the custom-set-variables form in the custom-file, so it will be loaded the next time Emacs starts.
  • The user still has the option to write the lisp in their configuration if they choose.

For the latter option:

  • We get the same behavioral advantages, ie the hooks set as desired on startup.
  • Nothing is saved in the custom-file because the configuration is functional instead.
  • The user must write the lisp in their configuration explicitly, but they are probably already doing that to use Crafted Emacs anyway, so it wouldn't be unexpected.

Regardless which option is developed, the documentation will need to be updated to reflect the change as well - see the docs/crafted-ide.org file

crafted-ide-config will configure eglot-ensure out of the box, but only for LSP binaries that exist on the machine (e.g. via executable-find)

This is an interesting idea, but it might be quite challenging to maintain long term as we would necessarily need to update executable names, so we would have to track that for every LSP. Not the end of the world, lsp-mode does most of the work for us for this (sort of) by tracking all the servers on their site - maybe even in the lsp-mode code as well. So, in spite of liking the idea, I think the trade-off on effort making this a larger lift reduces its attractiveness.

The user opts into eglot-auto-detect-all or similar to configure eglot-ensure for all eglot-server-programs. This works similarly to the tree sitter configuration in that it requires the user to call the function in their config after crafted-ide-config is loaded.

Yes, I agree with this. One alternative to this option is to provide a defcustom with an initializer that fires when the option is set.

from crafted-emacs.

mgmarlow avatar mgmarlow commented on August 20, 2024

This is an interesting idea, but it might be quite challenging to maintain long term as we would necessarily need to update executable names, so we would have to track that for every LSP. Not the end of the world, lsp-mode does most of the work for us for this (sort of) by tracking all the servers on their site - maybe even in the lsp-mode code as well. So, in spite of liking the idea, I think the trade-off on effort making this a larger lift reduces its attractiveness.

I experimented with a patch here to POC what this might look like: eedd8b9

from crafted-emacs.

jeffbowman avatar jeffbowman commented on August 20, 2024

huh... well, that's way easier than what I was expecting. Have you tested that patch? (probably a silly question)

EDIT: LOL, so you put that in your PR... a POC (to me) is a possible solution that may not have been tested much past the check to make sure the syntax is correct and no compile errors occur... I assume, since it is in your PR, that you have actually tried this code out and it works for you... I'll pull it into a VM and test a bit with it. Thanks for working on it though!

from crafted-emacs.

jeffbowman avatar jeffbowman commented on August 20, 2024

Tested your POC, unsurprisingly it works well... it was hard to tell if it worked because there is no output either way, but then too, that's just fine. I installed exactly one server and started Emacs, installed all the tree-sitter grammars, restarted Emacs and opened a python file.. and it failed... but apparently pyls isn't very good quality. Switched to pyright which worked immediately. Opened a go file and... nothing... no errors from eglot. Checked the go-mode-hook and go-ts-mode-hook, both were nil. Checked both python-mode-hook and python-ts-mode-hook and both had (eglot-ensure).

Thanks for the PR and if you don't mind updating the docs, then I'll merge it. (also mentioned on the PR itself).

Thanks for your work on this!

from crafted-emacs.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.