weavejester / cljfmt

A tool for formatting Clojure code
Eclipse Public License 1.0
1.11k stars 120 forks source link

Support access via deps.edn git coordinate #288

Closed spencerwilson closed 1 year ago

spencerwilson commented 1 year ago

Hello, Thanks for your work on cljfmt!

Problem

I'm eager to get the changes in https://github.com/weavejester/cljfmt/pull/274 but they're not released yet, so I changed tack and attempted a change like the following in my project:

-           :extra-deps {cljfmt/cljfmt {:mvn/version "0.9.0"}}}
+           :extra-deps {io.github.weavejester/cljfmt {:deps/root "cljfmt" :git/sha "cf708becf6e202ef0bc8c635aebbc49be7cd1c61"}}}

That didn't work though, since tools.deps doesn't support as dependencies packages that use Leiningen's "project.clj" as their manifest: https://github.com/clojure/tools.deps/blob/6ae2b6f71773de7549d7f22759e8b09fec27f0d9/src/main/clojure/clojure/tools/deps/extensions.clj#L21

As a result, the Clojure CLI tools don't know how to resolve the above dependency:

% clj -M:cljfmt --help
Error building classpath. Manifest file not found for io.github.weavejester/cljfmt in coordinate {:deps/root "cljfmt", :git/sha "cf708becf6e202ef0bc8c635aebbc49be7cd1c61", :git/url "https://github.com/weavejester/cljfmt.git"}

Suggested solution

Maybe have both a project.clj and deps.edn? The former would remain authoritative for development on cljfmt, whereas the latter would be for the benefit of teaching tools.deps what it needs to know to load the source tree as a dependency. This approach risks drift between the two manifests, and would increase maintenance burden.

spencerwilson commented 1 year ago

Ah, @seancorfield suggested a workaround here: https://ask.clojure.org/index.php/8507/failure-on-github-import-via-deps

spencerwilson commented 1 year ago

Workaround didn't actually suffice: readresource macroexpansion here e.g., https://github.com/weavejester/cljfmt/blob/9de4fab1dfac5c6c05d574dede670a823d499265/cljfmt/src/cljfmt/core.cljc#L292

would fail due to the indents resources not being on the classpath. I'm not sure if it's tools.deps who, when processing a git dependency, isn't putting the resources on the classpath that it sets up, or what. Am somewhat out of my depth here and had to move on.

weavejester commented 1 year ago

Fixed by #293.

borkdude commented 1 year ago

Indeed:

 bb -Sdeps '{:deps {cljfmt/cljfmt {:git/url "https://github.com/weavejester/cljfmt" :git/sha "7dfd55d5dd0756f30311a90f206c2dd32e56d18b" :deps/root "cljfmt"}}}' -m cljfmt.main

works fine now.

nihilismus commented 1 year ago

Thanks a lot for all the effort in improving this! :rabbit: :+1:

Note: at this moment there is not a new release-tag, the current one is 0.9.2, so the only way to use this in deps.edn is with a commit (full sha) after #293 was merged:

:cljfmt-check {:extra-deps {io.github.weavejester/cljfmt {:git/sha "3418b7f4c82c2a5acebc3c41e50138ea80d17010" :deps/root "cljfmt"}}
                 :main-opts ["-m" "cljfmt.main" "check"]}

Having just the commit does not help to get the idea of what version one is using :upside_down_face: