This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2021-09-02
Channels
- # aleph (25)
- # announcements (17)
- # aws (2)
- # babashka (72)
- # beginners (44)
- # calva (6)
- # cider (3)
- # clj-kondo (109)
- # cljfx (1)
- # cljsrn (31)
- # clojure (151)
- # clojure-austin (1)
- # clojure-europe (36)
- # clojure-nl (5)
- # clojure-norway (2)
- # clojure-spec (17)
- # clojure-uk (12)
- # clojurescript (74)
- # cursive (57)
- # data-science (1)
- # datascript (28)
- # datomic (40)
- # depstar (15)
- # gratitude (3)
- # helix (3)
- # introduce-yourself (1)
- # joker (1)
- # kaocha (2)
- # leiningen (2)
- # lsp (70)
- # lumo (2)
- # malli (2)
- # meander (4)
- # off-topic (10)
- # polylith (27)
- # quil (4)
- # re-frame (18)
- # reagent (24)
- # ring (4)
- # rum (1)
- # shadow-cljs (102)
- # sql (2)
- # tools-deps (48)
- # web-security (8)
- # xtdb (5)
Continuing from #tools-deps, where should I look to see about integrating with clj-kondo's analysis to get information out of vars?
@suskeyhose Let me get back to you on that in 15 minutes or so
no rush
So I'm taking a peek at the analysis folder and it looks like only specific metadata is exported, and for my usecase I'd need the :style/indent
metadata to be exported as well. Possibly a very small change. In any case I need to keep reading.
@suskeyhose This is correct. But! There is an issue to collect arbitrary metadata and the fix for this is pretty easy, so perhaps you could provide some input and we can finally implement this. https://github.com/clj-kondo/clj-kondo/issues/1280
I would be ok with just adding all metadata on a :meta
key or so, when providing an option. The only "danger" is that metadata contains stuff that isn't readable as EDN or so.
Ah, I see, since the data is passed over stdout it could cause issues.
Right, that's what I'd want to use it as.
but it'd be nice if we supported both use cases at once: the EDN output should preferably be valid.
OR we could make the selection of metadata optional (like in the issue) and then any EDN errors are the problem of the caller.
or we could choose to only export metadata with keywords as keys and constants as values
I think having the :var-meta
key like in the config is valid, and make it so just having true
for it will return everything.
I see.
so if the metadata has ^{:foo "bar"}
then we'd export :foo "bar"
but when it's :foo (+ 1 2 3)
we don't, for example
I think a valid constraint to put on it for now is to only grab the constant keys and values, and provide nothing for these dynamic ones, with the potential in the future that these may be added.
Just entirely excluding these dynamic ones would help prevent breaking changes by leaving the possibility they're added later, without giving in the meantime a quoted version of it for people to depend on.
so the relevant spot where this data is added is somewhere here: https://github.com/clj-kondo/clj-kondo/blob/21de6f46e30c119e82b847330cda5394247e4d36/src/clj_kondo/impl/analysis.clj#L38
I think attrs would need an extra :meta
key on which we do some processing before we add it to the analysis, if the option {:var-definitions {:metadata true}}
is given, I think
I think that would solve all the needs that I have, being able to start with the -T classpath with just .
and my own dependencies on the classpath, and then using kondo to get all the metadata to generate the config file.
Are there any other hangups I gotta be aware of before I start planning out my tool properly? Like having to figure out how to procure deps or something for kondo to lint them? Or will I be good just using tools.build's facilities for starting a clojure subproces to get the classpath with a set of aliases?
awesome
Updated the issue with a summary here: https://github.com/clj-kondo/clj-kondo/issues/1280#issuecomment-911945341
@suskeyhose Would you like to have a stab at this?
Yeah, I'll take a look at it tonight to see what I can get going.
Okay, so I'm looking at this today @borkdude, and it looks like the line you pointed me to will allow a meta key to come from whatever is passed to reg-var!, so does that mean the rest of this implementation is to just go through all the usages of reg-var! and make them include metadata? It seems like that's the obvious direction to go.
Yeah, so there's the namespace/reg-var!, and there's analysis/reg-var!, and it looks like the namespace one is the only one used, and it calls to the analysis one, so I just gotta update the namespace one I think.
'cause that already gets the metadata
Hmm. There's a lot of metadata in there that's coming from clj-kondo and not from the original var. Is there a way to select just the stuff coming from the source?
Actually I think I figured it out
This is still a "draft" PR, but I'd love to know if this is the right direction, or if I need to be doing something drastically different. I'm thinking then I can have the analysis/reg-var! do the actual filtering of which meta values are allowed, so that no code changes are needed elsewhere if we expand which kinds of meta are allowed.
Would be possible to clj-kondo correctly understand a var imported from potemkin and avoid unresolved-var
lint?
Hum, we have this lib at nubank that uses potenkim a lot, and when using the vars from its api, I'm getting unresolved-var :thinking_face:
I found the time to repro the issue in a separated repo/branch :) More details here: https://github.com/clj-kondo/clj-kondo/issues/1370
https://github.com/marick/Midje/tree/master/test-resources/clj-kondo.exports/marick/midje
So this works:
(ns clojure-sample.api
(:require
[clojure-sample.impl.foo]
[potemkin :refer [import-vars]]))
(import-vars
[clojure-sample.impl.foo some-func])
there is this lib at nubank that uses a lot of import-vars with full qualified symbols
I imagine that is just make clj-kondo lint this other way of import-vars I think right
So this is the code:
(defn analyze-import-vars [ctx expr]
(let [ns-name (-> ctx :ns :name)
import-groups (next (:children expr))]
[{:type :import-vars
:used-namespaces
(for [g import-groups
:let [[imported-ns & imported-vars] (:children g)
imported-ns-sym (:value imported-ns)]]
(do (doseq [iv imported-vars]
(let [iv-sym (:value iv)]
(prn :reg-var ns-name iv-sym expr {:imported-ns imported-ns-sym
:imported-var iv-sym})
(namespace/reg-var! ctx ns-name iv-sym expr {:imported-ns imported-ns-sym
:imported-var iv-sym})))
imported-ns-sym))}]))
instead of destructuring in the let, first check if it's a fully qualified symbol token
Managed to find a time :) https://github.com/clj-kondo/clj-kondo/pull/1371
One thing is, I would use qualified-symbol?
instead of qualified-ident
. Is there a reason you chose the latter?
I see, the ident checks for keyword as well while the other checks if it's a symbol only
https://github.com/clj-kondo/clj-kondo/commit/13066e47b8418072edc15a853164a2ef73ddc75a
With regards to the dynamic metadata stuff, is there anything that'd get in the way of checking if all the vars used in the definition are from clojure.core and then evaluating it with sci? I figure an eventual goal for this kinda thing would be to get something that's basically like constexpr evaluation that would just recursively check vars for sci compatibility and then eval them. Although for clarity, I'm not looking to implement this at all for the part I'm looking at tonight. I still want to stick to filtering metadata to constants.
> is there anything that'd get in the way of checking if all the vars used in the definition are from clojure.core and then evaluating it with sci? yeah, I think you can do that, unrelated to the metadata feature I think
Fair enough. That'll be something that I might think about in the future once the basics of general metadata are into analysis
Although at that point it might be worth just putting it into a tracking issue to see if there's interest in it before the work gets put in.
to get ^{(+ 2 2) (+ 4 4)}
metadata working
Like you mentioned before
I really do think these are rare cases though, so perhaps some exploration would be good to see often this occurs
Yeah, that's why I was thinking about a tracking issue to see for demand
just evaluating random expressions isn't safe to do even with SCI, e.g. you can have (defn ^{:foo (range)} dude [])
so I wouldn't recommend it
I think leaving it out for now is the best choice because it leaves us many options for what to do with it in the future.
I'd rather provide something for 80% of cases, and for the 20% provide nothing, than provide something for 100% of cases with a potential for breaking changes to 20% of it.
(although obviously the actual percentage of this kind of thing being uses is lower than that)