Skip to content

feat: clj-kondo defc linter#10

Open
rschmukler wants to merge 1 commit intothheller:masterfrom
rschmukler:rs/clj-kondo-hook
Open

feat: clj-kondo defc linter#10
rschmukler wants to merge 1 commit intothheller:masterfrom
rschmukler:rs/clj-kondo-hook

Conversation

@rschmukler
Copy link
Copy Markdown

Introduce a linter for the defc macro. It has the following features:

  • Ensure that a defc has a render definition
  • Ensure that only valid hooks can be defined inside defc (ie. hook, render, bind, event)
  • Ensure that events are defined with valid arity
  • Provide information to clj-kondo so that symbols from bind can be linted (eg. unused vars)

Ultimately this could be extended with other things (eg. ensuring that definitions are provided for events specified in render functions, linting hiccup, ensuring render uses <<, etc.) but this felt like a good-enough start.

linter_example

@thheller
Copy link
Copy Markdown
Owner

I have never used clj-kondo, so I have no idea what this is or how it works.

It'll likely take some time before I get a chance to figure this out. I like the idea though.

@rschmukler
Copy link
Copy Markdown
Author

Sure! I've done a fair bit of work w/ clj-kondo hooks so feel free to ask (here or Clojurians slack) if you have any questions or want any specific functionality.

@zeitstein
Copy link
Copy Markdown
Contributor

zeitstein commented Jul 14, 2023

This is great and something I've been putting off doing for a long time :) Thank you!

Some observations:

  • << can be used in place of render, so that should be supported.
  • Technically, event can be defined with arity 1-3. I use this often, to avoid e.g. (event :e [env _ _]). (It seems it will compile even with (event :e [] ...), though I then get an error related to get_hook_value when I trigger the event.)
  • Probably not important, but there is an expected ordering to hooks, iirc. Something like binds need to be placed before render/<<, event hooks can go both before and after.

After making minor modifications for the first two points above, my project is linted correctly. Thanks again!

@rschmukler rschmukler force-pushed the rs/clj-kondo-hook branch 3 times, most recently from ee72ff4 to ea6272e Compare July 14, 2023 15:38
@rschmukler
Copy link
Copy Markdown
Author

@zeitstein Thanks for the feedback! I have updated the PR to handle << and allow for arity 1-3 on event. I also added the order detection on ensuring that there are no binds after render.

@rschmukler
Copy link
Copy Markdown
Author

rschmukler commented Dec 15, 2024

Hey @thheller I wonder if you might reconsider merging this for those of us that do use clj-kondo - I just rebased it in case you're interested.

@rschmukler rschmukler force-pushed the rs/clj-kondo-hook branch 3 times, most recently from 9ed594c to be660de Compare December 15, 2024 23:57
@rschmukler
Copy link
Copy Markdown
Author

I ended up updating this to also support effect hooks with the following:

  1. Checks that effect's "when" is either: :mount, :render, :auto or a vector of dependencies.
  2. Checks that the second argument to effect is an rarity 1 binding vector

@rschmukler
Copy link
Copy Markdown
Author

Also @thheller if you still aren't familiar with clj-kondo but want to see what this does... In a project with theller/shadow-grove pointed at my branch, you can run:

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

Followed by

clj-kondo --lint src/

And you will get error messages for invalid usage of shadow-grove

@thheller
Copy link
Copy Markdown
Owner

I still have not looked into how clj-kondo works, or rather how it discovers these files. Do they actually need to be in the library .jar itself or just on the classpath?

I don't mind merging this, but I'd rather know what I'd get myself into beforehand. Also if anything skip the resources directory and move it into src/main instead.

Introduce a linter for the `defc` macro. It has the following features:

- Ensure that a `defc` has a `render` definition
- Ensure that only valid hooks can be defined inside defc (ie. `hook`,
  `render`, `bind`, `event`)
- Ensure that events are defined with valid arity
- Provide information to clj-kondo so that symbols from `bind` can be
  linted (eg. unused vars)

Ultimately this could be extended with other things (eg. ensuring that
definitions are provided for events specified in render functions) but
this felt like a good-enough start.
@rschmukler
Copy link
Copy Markdown
Author

@thheller clj-kondo works by looking for clj-kondo.exports/<org>/<project> on the class path of the library. Here's the section in the docs if you want to read more. Otherwise, I've moved it into main as requested.

@stuartrexking
Copy link
Copy Markdown

@rschmukler This is useful, but perhaps put this into it's own repository so it can be used only by those using clj-kondo.

There is no need for this to be in the shadow-grove repo right?

@rschmukler
Copy link
Copy Markdown
Author

The advantage of merging it into the repo is that it can be used by clj-kondo automatically just by virtue of having the dependency of shadow-grove in your project.

This is somewhat common practice, eg. manifold exports clj-kondo

@thheller
Copy link
Copy Markdown
Owner

FWIW the problem isn't merging this. The problem is maintaining this over time. I have no interest in doing that, since I'm unlikely to ever use this myself. Also most of the things this checks, the compiler already checks and will warn/error out itself, so I'd just get the same infos during compilation wihout an extra tool running. If the compiler errors are worse then what clj-kondo gives then I'd rather make those errors better.

@rschmukler
Copy link
Copy Markdown
Author

rschmukler commented Jun 30, 2025

Do as you will @thheller - I'm happy maintaining my fork with it if you don't want to merge it. For me, I appreciate the feedback as I type, which is what is offered with clj-kondo - but I realize not everyone has the same tool chain. I wrote it for myself but figured I would share since clj-kondo is pretty popular in the ecosystem.

If the API does diverge, and nobody (eg. me) is willing to update it, you can just delete the hooks and be in no worse state than where you are right now.I personally think you need to obligate yourself to maintaining it to the same level of functionality as the library itself.

That being said, I'm not currently using shadow-grove in anything so I'm not sure if the hooks are even up to date with whatever the latest API is, and since it hasn't been merged, I haven't been following very closely myself.

@zeitstein
Copy link
Copy Markdown
Contributor

zeitstein commented Jul 9, 2025

I'm using this everyday and it is still up to date. The important thing is that it exists as an option. We should point it out to new users, wherever we point to. If @thheller decides to include it in the repo, I can also help maintain it.

@thheller One thing I noticed compared with the linter is the compiler doesn't warn on unused bindings in defc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants