weavejester / hiccup

Fast library for rendering HTML in Clojure
http://weavejester.github.io/hiccup
Eclipse Public License 1.0
2.66k stars 174 forks source link

Add clj-kondo support for defelem and defhtml #202

Closed Garmelon closed 1 year ago

Garmelon commented 1 year ago

Lints defhtml as defn and adds a custom hook for defelem because of the optional attribute map argument. Tested it in another deps.edn project using https://github.com/clj-kondo/clj-kondo/blob/master/doc/config.md#importing and it seemed to work fine.

Also configures the project such that clj-kondo --lint src/ works with this custom config. I did not try to fix any reported issues though.

Fixes #201

borkdude commented 1 year ago

PR lgtm

weavejester commented 1 year ago

Thanks for the patch. Can you give me a code example that would be flagged by clj-kondo without this change?

borkdude commented 1 year ago

@weavejester Any code that uses any of the vars created using defelem etc. will give you "Unresolved var: ...", if you've previously analyzed hiccup before.

Steps to reproduce:

deps.edn:

{:deps {hiccup/hiccup {:mvn/version "1.0.5"}}
 :aliases {}}
mkdir -p .clj-kondo # directory in which analysis is saved
clj-kondo --lint $(clojure -Spath) --dependencies # initialize analysis cache with hiccup on classpath
clj-kondo --lint - <<< "(require '[hiccup.form :as form]) (form/file-upload)"
Garmelon commented 1 year ago

The hook in particular is necessary because otherwise code like this would be flagged for having too many arguments:

(hiccup.form/text-field {:autofocus true} :username prev-username)
weavejester commented 1 year ago

Thanks for the reproduction steps. I can confirm I see the warning, but I still get the warning when pointing the dependency to the clj-kondo-support branch. It's possible that clj-kondo doesn't check :local/root repositories, or look in the $HOME/.m2 directory, but if so I'm not sure how to verify that this patch works correctly. Do you know how to test the fix?

borkdude commented 1 year ago

but I still get the warning when pointing the dependency to the clj-kondo-support branch

You have to run:

clj-kondo --lint $(clojure -Spath) --copy-configs --dependencies

to copy the bundled configuration. After that, the warning will disappear.

weavejester commented 1 year ago

Thanks, I think we're getting closer, but I'm still getting a warning:

$ rm -rf .clj-kondo .cpcache && mkdir .clj-kondo
$ clj-kondo --lint $(clojure -Spath) --copy-configs --dependencies
Configs copied:
- .clj-kondo/hiccup/hiccup
$ clj-kondo --lint - <<< "(require '[hiccup.form :as form]) (form/file-upload :foo)"
<stdin>:1:36: warning: Unresolved var: form/file-upload
linting took 10ms, errors: 0, warnings: 1

This is using the deps.edn file:

{:deps {hiccup/hiccup {:local/root "../../path/to/hiccup"}}
 :aliases {}}
borkdude commented 1 year ago

Try linting the deps once again. The config should be in place in order for the defelem, etc to be properly interpreted.

borkdude commented 1 year ago

To only copy configs we actually have another flag, so once again everything in order:

mkdir -p .clj-kondo # directory in which analysis is saved
clj-kondo --lint $(clojure -Spath) --dependencies --copy-configs --skip-lint # preparation, only copy confings
clj-kondo --lint $(clojure -Spath) --dependencies # initialize analysis cache with hiccup on classpath
clj-kondo --lint - <<< "(require '[hiccup.form :as form]) (form/file-upload)"
borkdude commented 1 year ago

Note that when using a tool like clojure-lsp all of the above happens automatically.

weavejester commented 1 year ago

Perfect, thanks. So the full reproduction steps are:

Create a deps.edn file:

{:deps {hiccup/hiccup {:local/root "../path/to/hiccup"}}}

Then run:

rm -rf .clj-kondo .cpcache  # remove any old cache and configs
mkdir -p .clj-kondo # directory in which analysis is saved
clj-kondo --lint $(clojure -Spath) --dependencies --copy-configs --skip-lint # prep: only copy configs
clj-kondo --lint $(clojure -Spath) --dependencies # initialize analysis cache with hiccup on classpath
clj-kondo --lint - <<< "(require '[hiccup.form :as form]) (form/file-upload :foo)"

Using these steps, I get 1 warning for master, and 0 warnings for the branch of this PR.

borkdude commented 1 year ago

Correct