Fork me on GitHub
#clj-kondo
<
2021-09-19
>
ericdallo14:09:57

I think I found one more potemkin corner case that I think we should consider on fixing it on clj-kondo 馃У

ericdallo14:09:53

you can pass the first arg as the alias of the namespace and then the symbols

ericdallo14:09:12

I found that a lot of things doesn't work on metabase because this issue

ericdallo14:09:25

it should not be hard to support it I think

ericdallo14:09:25

not sure though how to get the namespaces from the alias on clj-kondo code at that time

borkdude15:09:03

ah right so an alias instead of the full namepace right?

ericdallo15:09:37

Also, it's possible to pass multiple vectors, but I think kondo is handling those already

borkdude15:09:55

ok, I'll take a look

borkdude15:09:23

@UKFSJSM38 should be fixed on master

ericdallo15:09:55

oh that was fast :)

ericdallo15:09:00

thanks, I'll will try

borkdude16:09:18

clj-kondo-2021.09.16-20210919.160121-5

ericdallo16:09:21

it worked! thank you! FYI @U11BV7MTK this should help with a lot of false positives on metabase , probably the rest is just macros that should be configured on clj-kondo side with lint-as and custom hooks :)

ericdallo16:09:42

I'll include this clj-kondo bump on the next clojure-lsp release, thanks for the help @U04V15CAJ

馃憤 2
ericdallo23:09:42

One more potemkin issue 馃槄 now related to kondo configuration 馃У

ericdallo23:09:12

using the metabase as example again, I found a issue related to custom configuration like :lint-as for macros that are being used by potemkin

ericdallo23:09:13

For example: metabase has https://github.com/metabase/metabase/blob/master/test/metabase/test/data.clj#L204 dataset macro that I want to lint-as def . I added this to the .clj-kondo/config.edn :

{:lint-as {metabase.test.data/dataset clojure.core/def}}
but that doesn't seems to work for the usages that use potemkin under the hood, for example https://github.com/metabase/metabase/blob/master/test/metabase/query_processor/pivot_test.clj#L86. This only works if I change the lint-as to the namespace that import the vars with potemkin like:
{:lint-as {metabase.test/dataset clojure.core/def}}

ericdallo23:09:59

The user could add both namespaces to the lint-as is just that seems something clj-kondo could be smart and already check for the config for that imported potemkin var, right? Also, the same happens for hooks configuration

ericdallo23:09:39

Sorry for bringing a lot of bugs, is just that I found metabase a good project to find/test LSP and see what doesn't work/could be improved :)

borkdude09:09:30

I agree this would be clever but this is a little bit complicated

borkdude09:09:42

how many macros is this about

borkdude09:09:34

we need to read the cache in an earlier phase to support this so it's not a trivial change

borkdude09:09:23

I added a TODO test: 0ae0e51158570d3c94be7c0c7b16c0c8cc593ecb You're welcome to make an issue about this but I don't consider this high priority for now (weighing the complexity vs how much effort it is to configure this)

ericdallo12:09:32

Sure, that's fair, I only found 3 macros until now in the metabase that fall into this issue, but I agree this is no high priority

pithyless11:09:41

FWIW, I wrap a lot of library macros and functions this way (the indirection is useful if you want to augment or modify the original behavior systematically throughout your application) and sometimes end up adding both the wrapper and the original macro to clj-kondo. It's not pretty, but it has not been a deal-breaker so far.

pithyless11:09:42

^ I don't think this is inherently related to potemkin; but I may be wrong and some of my config linters are no longer necessary.

borkdude11:09:03

I'm not sure what you meant by the last sentence: you no longer need config which you needed before?

pithyless13:09:39

{:lint-as
{metabase.test.data/dataset clojure.core/def
 metabase.test/dataset clojure.core/def}}
^ I was thinking that ideally one of these would be enough, and the other could be inferred.

borkdude13:09:17

yes, that was what this discussion was about.

pithyless13:09:45

Yep, I wanted to just add a datapoint that I come across this problem in my own projects, it'd be nice if it were fixed, but it's not something I consider a very thorny issue that is a pain to deal with it. If that was not obvious, sorry for adding to the noise. :)

borkdude13:09:16

thanks for the input. it requires an architectural change in clj-kondo, it might be fixed one day, welcome to make issues, but low priority for now

borkdude14:09:49

I wonder if would be useful to have a config to make clj-kondo clear if two things should be treated the same, but this is already what :lint-as is for more or less ;)