jcollard / elm-mode

Elm mode for emacs
GNU General Public License v3.0
373 stars 67 forks source link

Add support for project.el #169

Closed theothornhill closed 4 years ago

theothornhill commented 4 years ago

When using Eglot (and I assume lsp-mode if you choose to opt out of its own root-finder) I fail to find root. This PR adds support for emacs' own project.el, such that we get auto-detect of root.

I've made an assumption that "elm.json" exists in root. I am not sure whether this is a sound assumption. We could maybe add a defcustom or something to handle this.

theothornhill commented 4 years ago

Ah I see project.el requires emacs 26.3. I guess this is an incopatible change, then?

purcell commented 4 years ago

I've used elm-mode with eglot and elm-language-server without needing to make such a change. What was the problem you were seeing?

theothornhill commented 4 years ago

I assume that is because you probably use projectile.el or something like that?

To reproduce in emacs -Q:

(package-initialize)
(require 'eglot)
(require 'elm-mode)

When I M-x eglot in a buffer in a newly initiated elm project with elm init, jsonrpc returns [jsonrpc] Server exited with status 7

Adding some project handling like this seems to fix it on my end, though! But it might not be the root cause :) ?

purcell commented 4 years ago

I think the reason I haven't encountered this is because I've never run eglot without being in a VC root. If we modify project-find-functions for users, then the project-root for a larger project which contains some Elm code will be the location of the elm.json, even if that is in a subdirectory: I've seen this setup before. In your case, that's the behaviour you'd want for eglot, but not for project-find-file and the general goals of project.el. So there's a mismatch here, and we can't get it right for everyone, I think: I'm not sure using project-root is what eglot should be doing, tbh, or at least it should be possible to override how the root is found for each language server.

Probably the best option is to simply provide an elm-project-find-function function and document how the user can add it to project-find-functions if they want.

purcell commented 4 years ago

P.S. locate-dominating-file is the magic function, btw: see 3f10c61

theothornhill commented 4 years ago

Yeah, seems right! Good point with the “big monorepo” case here. I guess we can easily close this!

And thanks for locate-dominating-file :)

purcell commented 4 years ago

Yeah, seems right! Good point with the “big monorepo” case here. I guess we can easily close this!

Great, I'll do that then. Thanks for bringing this up.

And thanks for locate-dominating-file :)

The number of times I've used that function, you wouldn't believe. And I've also seen at least 20 reimplementations of it. Not a very obvious name tbh.