clojure-lsp / clojure-lsp

Clojure & ClojureScript Language Server (LSP) implementation
https://clojure-lsp.io
MIT License
1.18k stars 155 forks source link

Better project-path default configuration behaviour #1366

Open lispyclouds opened 2 years ago

lispyclouds commented 2 years ago

Is your feature request related to a problem? Please describe. As per this slack thread in order to make clojure-lsp prefer deps.edn over project.clj, I should be able to specify the :project-path to point to just deps.edn and it should ignore project.clj.

Describe the solution you'd like As mentioned in https://clojure-lsp.io/settings/#classpath-scan I'd like to set this in .lsp/config.edn:

{:project-specs [{:project-path "deps.edn"}]}

Currently I also need to provide a :classpath-cmd here as well, but my intention being just to point to deps.edn specifically, that should default to ["clojure" "-A:dev:test" "-Spath"] when not provided.

This makes it simpler to configure and not worry about this internal detail.

Describe alternatives you've considered None

Additional context None

lispyclouds commented 2 years ago

When looking at the source, I do see the complexity in implementing this. I believe we need to do a merge of % and default-project-specs here.

But the merge needs to be smart about only finding the relevant map from the defaults where the :project-path matches and the inject the default :classpath-cmd? It's harder as its a vec of things and also the :project-path could be whatever not specifically just "deps.edn".

Is my understanding right? Is there a better way to do this?

lispyclouds commented 2 years ago

I was thinking the merge could check for "deps.edn" as a substring match in the ones provided by the user and if that matches, we inject the default :classpath-cmd maybe?

ericdallo commented 2 years ago

Yep, pretty tricky, I was thinking if we should just leave current project-spec as it is and deprecate in favor of a new setting thinking better those cases, something more simple with good defaults, WDYT? do you have anything in mind as a new better behavior?

lispyclouds commented 2 years ago

Yes, I would second a better and cleaner implementation deprecating this one. I suppose the config being a vec of maps is to have some form of priority? Would be nicer I think if its just a map maybe? something like this, showing the defaults:

{:project {:tools-deps {:path "deps.edn" :classpath-cmd [...]}
           :leiningen  {:path "project.clj" :classpath-cmd [...]}
           ...}}

This for me seems to be a more intuitive config and I think would be easier to merge with user provided config? Each part of the conf could be overriden and merged with in clojure-lsp's startup? Not sure if we'd wanna handle tool priority maybe?

ericdallo commented 2 years ago

Looks better, maybe rename project to classpath-discovery? We would need to handle projects using legacy config and this one, taking precedence the ones that have only project-specs but if have the new config it´d override any project-specs I think.

This setting would be used only during startup since it's used on clojure-lsp.startup, I don't think we need auto-refresh for this setting so, less complexity.

lispyclouds commented 2 years ago

:classpath-discovery seems better and then I think we can use :cmd instead of :classpath-cmd. Merging this map with the :project-specs vector is still a bit complex I think. But in the end this should be a better solution for me!

ericdallo commented 2 years ago

Agreed, I would go via a simpler idea of just which one was passed project-specs or classpath-discovery and using it, if both were passed, use classpath-discovery, avoiding merging both configs or something like that

lispyclouds commented 2 years ago

Makes sense!