tolitius / mount

managing Clojure and ClojureScript app state since (reset)
Eclipse Public License 1.0
1.22k stars 88 forks source link

Add clojure-kondo for mount.defstate #130

Closed rajcspsg closed 5 months ago

rajcspsg commented 1 year ago

closes https://github.com/tolitius/mount/issues/128

tolitius commented 1 year ago

thanks for the PR!

I use vi for development, hence the question for vs code, intellij, emacs humans:

would it work if this does not live under the src, but under the resources instead (as per kondo docs)?

rome-user commented 1 year ago

@tolitius This should indeed be in the resources folder. The src folder is not the best path for this.

FWIW, I use Emacs without LSP, so this issue has never occurred to me, but I run linters in CI, and clj-kondo will complain about the defstate. Also, I think def-catch-all may be better for linter here.

rajcspsg commented 1 year ago

Thanks @rome-user @tolitius let me update the PR with config shortly

ieugen commented 5 months ago

@rajcspsg : Can you please update the file path? @tolitius : Would be great if we get this in mount . The change is quite small, if @rajcspsg does not reply in a timely manner, can you please edit the commit and merge ?

I can resubmit the patch - but would like for the original author to get recognition.

Similar solution: https://gist.github.com/ethpran/e1741a5c408aec831b5e3a7e24f40fea

Thanks

rajcspsg commented 5 months ago

@ieugen @rome-user @tolitius Sorry for the delay. I updated the PR. Please let me know if you have any other comments

tolitius commented 5 months ago

great thanks! 🤘

it should be in "[mount "0.1.18"]"

let me know whether it works as expected

ieugen commented 5 months ago

I've updated to 0.1.18 and removed my clj-kondo config but Calva reports errors:

image

Before this I had this line in my clj-kondo, which worked (no linting errors) .


{:lint-as {mount.core/defstate clojure.core/def}}
ieugen commented 5 months ago

I followed the instructions here: https://github.com/clj-kondo/clj-kondo/blob/master/doc/config.md#importing

And it still did not copy the config.

@tolitius : I believe we need to add "resources" dir to the source paths. See https://github.com/clj-kondo/clj-kondo/blob/master/doc/config.md#sample-exports

Includes "resources" in its deps.edn :paths so that the exports will be included on the classpath when the repo is included as a :git/url dependency.

Files are not in jar:

$ tree $PWD
/home/ieugen/.m2/repository/mount/mount/0.1.18
├── META-INF
│   ├── MANIFEST.MF
│   └── maven
│       └── mount
│           └── mount
│               ├── pom.properties
│               └── pom.xml
├── mount
│   ├── core.cljc
│   └── tools
│       ├── graph.cljc
│       ├── logger.cljc
│       ├── macro.cljc
│       └── macrovich.cljc
├── mount-0.1.18.jar
├── mount-0.1.18.jar.sha1
├── mount-0.1.18.pom
├── mount-0.1.18.pom.sha1
└── _remote.repositories

Also, it would be nice to add deps.edn to the project so we can test this before cutting a new release.

$ clj-kondo --lint "$(clojure -Spath)" --copy-configs --skip-lint

Configs copied:
- .clj-kondo/hiccup/hiccup
- .clj-kondo/metosin/malli
- .clj-kondo/org.clj-commons/byte-streams
- .clj-kondo/potemkin/potemkin
linting took 143ms, errors: 0, warnings: 0
rome-user commented 5 months ago

Looks like the project uses boot to build the uberjar instead of Leiningen, that would explain why the resources folder wasnt bundled into the JAR. (Leiningen by default adds resources folder to the :resource-path. In boot, you must write code to do this yourself, and the build.boot file does not seem to do this).

tolitius commented 5 months ago

since I don't use "clj-kondo" it is difficult to know what exactly is the right approach in incorporate it in the library

I see this doc, but including a non compilable namespace in the jar is not an ideal solution:

(ns hooks.defstate
  (:require [clj-kondo.hooks-api :as api]))

for example this is what I get when I refresh namespaces with tools.namespace:

#error {
 :cause "Could not locate clj_kondo/hooks_api__init.class, clj_kondo/hooks_api.clj or clj_kondo/hooks_api.cljc on classpath. Please check that namespaces with dashes use underscores in the Clojure file name."
 :via
 [{:type clojure.lang.Compiler$CompilerException
   :message "Syntax error compiling at (hooks/defstate.clj:1:1)."

not ideal.

I added deps.edn to cover #131 since I do agree that with boot out of the picture "deps.edn" is deff more current, also allows people to contribute / play with the lib a lot easier.

I use "deps.edn" in a lot of other libs, but I do not quite like the maven feel to it (e.g. "-S:foo:bar -X:zoo:but:not -A:because:it:is:now -M) hence I always include a Makefile

so things can be as simple as (they were with lein and boot):

make repl
make jar
make test
...

in any case I did build "0.1.19-SNAPSHOT" with "resources" included in ":paths"

let me know whether it works for you

but I am still skeptical about keeping it in the classpath, since it would only compile for people who use "clj-kondo"

ieugen commented 5 months ago

hello,

hi @tolitius , I tested this and it partially works. Including the resources in the jar file does it's job, but there are some issues on the linting logic cc @rajcspsg .

but I am still skeptical about keeping it in the classpath, since it would only compile for people who use "clj-kondo"

I don't follow this. You can add a lot of files in the jar and they will not influence the code (think about META-INF stuff or the lib/ inlcusion for jars - that require an entry in manifest.mf to work ) They will be considered only if you require them or in this case, clj-kondo scans for them. They should be transparent to any well behaved code.

You probably get the above error because you need to add the clj-kondo library for development. IMO clj-kondo should be available while you work on the clj-kondo linting code, but it's not a dependency of the final jar.

since I don't use "clj-kondo"

You are definitely missing out :) . clj-kondo with lsp is like crack - you get addicted and Calva is a good IDE for Clojure. I am also experimenting with Emacs for Clojure but things are behind Calva for me at least (new to Emacs).

Regarding the linting logic issue:

In my code, I defstate conf and I pass that to jetty server handler but the symbol is not found.

image

Some caveats:

@rajcspsg : Do you think you can fix this easy? If so I propose you work on branch and I can test that before we submit to @tolitius for merge. Sounds ok to you?

NoahTheDuke commented 1 month ago

@ieugen I believe what you're seeing is a consequence of the resulting node passed to clj-kondo from the hook. For (defstate conf :start (load-config)), the hook outputs (fn* [conf] conf (load-config)) when it should output (def conf (do (load-config))). Fixing that should be relatively simple but will require another release.