clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

Provide clj-kondo hook for def-http-method #582

Closed DerGuteMoritz closed 2 years ago

DerGuteMoritz commented 2 years ago

This allows clj-kondo to properly lint aleph.http/post and friends. Similar case as https://github.com/ptaoussanis/encore/pull/56, for example (see there for some more explanation).

KingMob commented 2 years ago

I like this idea. I have a few (possibly naive, I'm no kondo expert) questions, though:

  1. Could it be done simpler, with something like :macroexpand or :lint-as?
  2. Should the directory path reflect aleph's group ID, which is also "aleph"? E.g., "resources/clj-kondo.exports/aleph/aleph/aleph/http.clj". Although, the example in https://github.com/clj-kondo/clj-kondo/blob/master/doc/hooks.md#transformation prefixes the kondo namespace with hooks, so maybe it shouldn't be identical? Not clear what's needed.
  3. The state-flow and midje examples that the encore PR mention don't have a .clj-kondo/config.edn, but this one and the encore PR do. Why is there a discrepancy? Are they not linting themselves with their own hooks?
  4. Finally...given any thought to all the other linter issues? Zach liked macros, and a lot of this code predates common clj styles and best practices, which creates a lot of false positives when linting.
DerGuteMoritz commented 2 years ago

I like this idea. I have a few (possibly naive, I'm no kondo expert) questions, though:

No problem, I'm not an expert by any means either, yet :smile:

  1. Could it be done simpler, with something like :macroexpand or :lint-as?

:lint-as is not possible AFAICT, but :macroexpand is! Thanks for the idea, that makes it much easier to grok. Also, I changed it so that it knows about the allowed arities for the defined functions, so one can now get something like this:

error: aleph.http/post is called with 3 args but expects 1 or 2

:raised_hands:

  1. Should the directory path reflect aleph's group ID, which is also "aleph"? E.g., "resources/clj-kondo.exports/aleph/aleph/aleph/http.clj". Although, the example in https://github.com/clj-kondo/clj-kondo/blob/master/doc/hooks.md#transformation prefixes the kondo namespace with hooks, so maybe it shouldn't be identical? Not clear what's needed.

Yeah I was a bit confused by that, too. I concluded that it doesn't really matter but maybe @borkdude can chime in on that :slightly_smiling_face:

  1. The state-flow and midje examples that the encore PR mention don't have a .clj-kondo/config.edn, but this one and the encore PR do. Why is there a discrepancy? Are they not linting themselves with their own hooks?

Yeah, apparently they're not using clj-kondo themselves. I figured it wouldn't hurt to include it as a starting point but I wouldn't mind removing it either. Your call!

  1. Finally...given any thought to all the other linter issues? Zach liked macros, and a lot of this code predates common clj styles and best practices, which creates a lot of false positives when linting.

Not yet, gotta admit that I mainly wanted to scratch my own immediate itch here :smile: But I'd be happy to expand the patch with fixes for all the remaining warnings and errors if you care!

borkdude commented 2 years ago

resources/clj-kondo.exports/aleph/config.edn

This needs one extra directory, e.g.:

resources/clj-kondo.exports/clj-commons/aleph/config.edn

So one directory for the organization and one directory for the library.

KingMob commented 2 years ago

Thanks for chiming in, @borkdude. If it's a maven thing, the groups for most of zach's old libs were changed to clj-commons, but to minimize disruption (for now), we left aleph and manifold untouched, so they're still aleph/aleph and manifold/manifold. If mvn coordinates don't matter, than "clj-commons" is probably the better group.

KingMob commented 2 years ago

@DerGuteMoritz

  1. If we don't have to follow maven coords, let's use clj-commons as the group. If we do, the group is aleph.
  2. Let's have a project-level kondo config.edn. I like kondo and want to make more use of it.
  3. If you have the time/energy. 😄 If not, the perfect shouldn't be the enemy of the good, we can start with this, and add more hooks as/when they annoy us.
borkdude commented 2 years ago

The two-level directory isn't related to maven at all. The reasons for this are: 1) avoiding conflicts, 2) clj-kondo auto-loads from the two-level directory, but not from one directory. The org can be arbitrary but usually reflects the git repo org or the mvn org.

DerGuteMoritz commented 2 years ago

@borkdude Thanks a lot for the clarification! I have applied the necessary changes and properly tested it by locally installing the jar and uising it in another project. Generally it seems to work now but two new questions came up:

  1. I had to rename the namespace providing the hook from aleph.http to something else. Otherwise the cache would only learn about my hook but none of the unhooked API. Is this expected behavior?
  2. After importing the config to a project via --copy-configs and adding the path to its :config-paths, I had to initialize the cache once more for the hook to have an effect. Before that, I would always get "warning: Unresolved var: http/post". I guess this is a special-case because the hooked macro here defines the var in question?

@KingMob After some further consideration, I think fixing all existing linter warnings and errors is better saved for a follow-up patch given the following result:

$ clj-kondo --lint src/ test/ examples/
[...]
linting took 560ms, errors: 116, warnings: 119

:grimacing:

borkdude commented 2 years ago

@DerGuteMoritz

  1. I'm not sure what the problem here was, but renaming the hook namespace isn't a problem, as long as you realize that auto-resolved symbols/keywords are resolved correctly in the macro.

  2. Unfortunately this is a chicken and egg problem. The config gets copied as a result of linting, but we already need it during linting. The solution here is to lint twice indeed. But with the config in place, that should not be necessary more than once. Projects are adviced to check in their copied configs into source control for consistent linting.

KingMob commented 2 years ago

Looks good to me.

borkdude commented 2 years ago

@DerGuteMoritz We are considering an API here to copy the configs in a first step (which should be relatively fast) so you can then lint with all the configs in place: https://github.com/clj-kondo/clj-kondo/issues/1625