Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clj-kondo hooks #56

Closed
wants to merge 1 commit into from

Conversation

ericdallo
Copy link
Contributor

This should make clj-kondo understand the defalias custom macro and correctly lint it as an alias, this PR also add the hook config to resources classpath following export feature, this make clj-kondo understand this macro on any lib that uses encore for example taoensso.timbre/spit-appender, the user only needs to add to its clj-kondo config a {:config-paths ["taoensso/encore"]}.

Some examples of other libs that did the same: midje, state-flow

@ericdallo
Copy link
Contributor Author

LMK if any questions @ptaoussanis :)

@ptaoussanis ptaoussanis self-requested a review November 9, 2021 07:43
@ptaoussanis ptaoussanis self-assigned this Nov 9, 2021
@ptaoussanis ptaoussanis removed their request for review November 9, 2021 07:44
@ptaoussanis ptaoussanis force-pushed the master branch 9 times, most recently from 8beb57c to 5fc3019 Compare April 20, 2022 07:52
@ptaoussanis ptaoussanis force-pushed the master branch 2 times, most recently from 7bec5eb to 1da01a9 Compare May 2, 2022 11:22
@ptaoussanis ptaoussanis force-pushed the master branch 4 times, most recently from d6af559 to d6236b6 Compare June 29, 2022 15:21
@ptaoussanis ptaoussanis force-pushed the master branch 2 times, most recently from ecae192 to ead76df Compare July 14, 2022 15:59
@ptaoussanis ptaoussanis force-pushed the master branch 6 times, most recently from abf7e8e to efe0346 Compare October 26, 2022 15:37
@ptaoussanis ptaoussanis force-pushed the master branch 8 times, most recently from b983a1c to 4a4855c Compare October 31, 2022 08:31
@ptaoussanis ptaoussanis force-pushed the master branch 3 times, most recently from 3992229 to 865a6ef Compare November 9, 2022 15:27
@ptaoussanis ptaoussanis force-pushed the master branch 5 times, most recently from 717ab7a to 4718d57 Compare November 21, 2022 18:02
@ptaoussanis
Copy link
Member

@ericdallo Hi Eric, apologies for the long delay getting back to you on this!
I've never used kondo, so not too familiar with the details on this.

Just double checking if the PR is still up-to-date / relevant?
If so, will get it merged as part of the next Encore release.

Thanks!

@ericdallo
Copy link
Contributor Author

No problem @ptaoussanis!
Yes, it's still relevant as it should help users of encore and clj-kondo (via clojure-lsp as well) have that config automatically and make linter and other features happy :)

@ptaoussanis
Copy link
Member

Merging now, thanks Eric!

@jumarko
Copy link

jumarko commented Dec 15, 2022

@ericdallo
I don't see where the dependency on clj-kondo is defined in the project.
In fact, I'm getting these errors in one of my projects (when loading taoensso.encore)

1. Caused by java.io.FileNotFoundException
   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.

@ptaoussanis
Copy link
Member

@jumarko Hi Juraj, while waiting for a reply from Eric - I'd like to ask in the meantime:

In fact, I'm getting these errors in one of my projects (when loading taoensso.encore)

What do you mean by "loading" here? Can you be more specific about exactly what command you're running? What build tool are you using?

Thanks!

@ericdallo
Copy link
Contributor Author

This works following clj-kondo exports feature, basically the encore lib how has under resources folder the clj-kondo hooks configuration, when clj-kondo is lining a project that has encore as the lib dependency, clj-kondo will scan the encore jar and find this clj-kondo config as it's available on the classpath, all what the user needs to do is include this on it's project:

.clj-kondo/config.edn

{:config-paths ["taoensso/encore"]}

@jumarko
Copy link

jumarko commented Dec 15, 2022

@ptaoussanis I'm trying to use datalevin that requires encore.
I had encore set in my deps.edn file explicitly but it was too old so I decided to upgrade to the latest version of encore.
However, when I required dataleving.core it failed (transitively) because of encore:

(ns clojure-experiments.datomic.datalevin
  "https://github.com/juji-io/datalevin/blob/master/README.md"
  (:require
   [datalevin.core :as d]))

trying to load the above ns definition fails:

Syntax error macroexpanding taoensso.encore/defalias at (taoensso/encore.cljc:541:6).
[encore/defalias] Failed to resolve source var

It works using encore version 3.30.0 (chosen more or less randomly looking at the releases that were made before this PR was merged).

I was wondering why it couldn't find the var and thus the namespace.
Thus I opened the encore jar and tried to load encore.clj - I didn't realize it was actually the one in clj-kondo.exports/...

That's the reason why I thought it's related to this change.
But it seems that problem is different - maybe it has something to do with the truss version in my project?

@ptaoussanis
Copy link
Member

Syntax error macroexpanding taoensso.encore/defalias at (taoensso/encore.cljc:541:6).

Just to confirm: this error should also mention what the exact var is couldn't be resolved. It's taoensso.truss/with-data?
If so, it sounds like you may have a dependency conflict involving Truss.

Are you manually including Truss in your own project? If so, please make sure that you're either on the latest version - or just let Encore bring in an appropriate version automatically.

Some info to help you debug here.

@jumarko
Copy link

jumarko commented Dec 15, 2022

Upgrading to truss 1.8.0 indeed fixed the problem.
Thanks for the help!

@ptaoussanis
Copy link
Member

No problem Juraj, thanks for the confirmation. And thanks @ericdallo!

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

Successfully merging this pull request may close these issues.

3 participants