Fork me on GitHub
#clj-kondo
<
2021-12-17
>
n2o09:12:39

Hi, thanks for the big release, but there seems to be a problem with re-frames subscriptions. I built a minimal example for this:

(require '[re-frame.core :as rf])

(defn- show-id [subscription]
  (let [id @(rf/subscribe subscription)]
    [:h4 id]))

(defn- ids []
  [show-id [:user/id]])

(rf/reg-sub
 :user/id
 (fn [db _]
   (get-in db [:user :id])))
So there is the component ids, which should show in this example some ids, doesn’t really matter. The subscription key [:user/id] is provided via an argument. But clj-kondo says there is an unused binding: src/main/schnaq/interface/user.cljs:56:17: warning: unused binding subscription

n2o09:12:48

The error message points to line 3, where subscription is the parameter of the component.

borkdude09:12:02

Thanks for the issue, I'll take a look. /cc @U0508JT9N

benedek09:12:23

hm… the problem could be that we don’t analyze the paramaters of the rf/subscribe apart from the special code which tries to figure out the subs id. so ‘ordinary’ clj-kondo misses the usage of the subscription local

borkdude09:12:24

yeah, I'm taking a look now

borkdude09:12:30

@U0508JT9N This looks suspicious:

[subscription-id & subscription-params] (:children (first (next (:children expr))))
first next?

benedek09:12:41

next gives you a coll and we want the first element, right?

benedek09:12:57

tbh we expect that first element to be a vector

benedek09:12:08

which not true here apparently

borkdude09:12:47

when I change this to

[subscription-id & subscription-params] (next (:children expr))
then it works again

borkdude09:12:19

ah gotcha, yes, we expected a vector here

borkdude09:12:35

@U1PCFQUR3 perhaps as a workaround you can use that syntax for now?

benedek09:12:55

i don’t think we handle the case when the subs param vector is passed in as a local as in the above bugreport repro example

borkdude09:12:17

I'll make a "vector" special case then

n2o09:12:50

Ok, so I’ll just wait for the special case? 🙂 Thanks

borkdude09:12:55

if you don't want to wait for the fix, I think it might be better if you passed the args to build the subscription instead of the subscription itself, so clj-kondo can see the keyword and static analysis improves by that as well, if in the future you want to find all references

n2o09:12:01

Ok sounds reasonable :thumbsup: thanks for your work

borkdude09:12:13

(defn show-id2 [x y z]
  (let [id @(rf/subscribe [:foobar x y z])]
    [:h4 id]))

borkdude09:12:20

but since it's already a subscription, I don't think you need to pass it through subscribe at all

borkdude09:12:26

you can directly deref it?

borkdude09:12:41

or do you pass the vector argument as subscription?

n2o09:12:37

We’ll pass the vector argument into the subscription

n2o09:12:39

Subscriptions can have arguments, which is why we pass the vector to respect the arguments. Sometimes the subscription key changes, but the provided data follows the same structure, which is why we provide the complete vector for re-frames subscription function

n2o09:12:07

So we can re-use the components more easily.

borkdude10:12:59

Fixed on master

👍 1
n2o10:12:05

Awesome 🥳

borkdude10:12:43

@U1PCFQUR3 Not sure if this is blocking anything, I'll wait for some more bug reports related to the new release and then will do a small release soon-ish

n2o10:12:01

We are fine with this, thanks!

borkdude10:12:48

you can use a temporary #_:clj-kondo/ignore on top of the problematic form

Marc O'Morain09:12:27

@borkdude today is an ironic day – I have been repsonsible for accidentally breaking 1000s of peoples builds on CircleCI over the years, but today you broke my build 😄 clj-kondo is warning that our definition of random-uuid is shadowing clojure.core/random-uuid , and we have warnings treated as errors.

Marc O'Morain09:12:05

Also, thank you for the new version of kondo and all the word you do on it!

Marc O'Morain09:12:55

We have a leiningen project that uses Clojure 1.10, and when we run clj-kondo it’s outputting this:

[clj-kondo] Auto-loading config path: babashka/sci
[clj-kondo] Auto-loading config path: babashka/fs
src/workflows_conductor/entities/uuid.clj:7:1: warning: random-uuid already refers to #'clojure.core/random-uuid
linting took 2223ms, errors: 0, warnings: 1

borkdude09:12:25

Is that not desired?

Marc O'Morain09:12:29

our script to invoke it is:

#!/bin/bash -eo pipefail
# Turn off errexit as we want to continue and catch the error code ourselves
set +e

lein clj-kondo --lint src
ret_code_src=$?

echo

# For tests we allow private-call; otherwise use the same config as src
lein clj-kondo --config '{:linters {:private-call {:level :off}}}' --lint test
ret_code_test=$?

# Now fail the job if *either* of the src or test
# invocations returned a non-zero error code
[ "$ret_code_src" -eq 0 -a "$ret_code_test" -eq 0 ]

Marc O'Morain09:12:50

random-uuid isn’t in clojure.core 1.10 right?

Marc O'Morain09:12:06

We’re going to add (:refer-clojure :exclude [random-uuid]) to our namespace

borkdude09:12:03

it's in 1.11 and clj-kondo is updated for clojure 1.11 already :)

borkdude09:12:22

but if you lint your dependencies, clj-kondo will automatically pick up 1.10 again

Marc O'Morain09:12:38

aha, that’s it!

jvtrigueros00:02:26

@U0K592YDP sorry to raise this thread, but I’m seeing this behaviour as well and am not sure what is meant by “lint your dependencies”, could you share how you got around this?

Marc O'Morain13:02:47

We worked around this like this by adding the following:

;; With clojure 1.11 alpha, the (random-uuid) function is introduced.
;; We don't have this yet, so let's silence the warning.
;; We may want to consider replacing our custom function with the built-in
;; one if we upgrade clojure.
#_{:clj-kondo/ignore [:redefined-var]}
(defn random-uuid
  "Returns a random type 4 universally unique identifier."
  []
  (UUID/randomUUID))

borkdude13:02:50

a different solution: (:refer-clojure :exclude [random-uuid])

robert-stuttaford09:12:04

apologies, my google-fu is failing me. how do i clj-kondo on an M1 Silicon mac?

n2o09:12:35

I installed it using homebrew.

robert-stuttaford09:12:58

on an M1 mac?

➜  bootstrap git:(master) ✗ brew install borkdude/brew/clj-kondo
Error: Cannot install in Homebrew on ARM processor in Intel default prefix (/usr/local)!

n2o09:12:49

Yes, but you have the same error as I had. If homebrew wants to install in /usr/local, homebrew is trying to install x64 programs.

n2o09:12:10

If configured correctly, homebrew installs arm-tools in /opt/homebrew.

n2o09:12:16

I had it misconfigured when I switched to an M1 mac, deinstalled it completely, installed it again and then it chose to install arm tools instead. Had it misconfigured, because I migrated via a time machine image from an x64 mac to an arm mac

robert-stuttaford09:12:49

softwareupdate --install-rosetta
arch -x86_64 brew install borkdude/brew/clj-kondo

robert-stuttaford09:12:58

this worked, thank you!

n2o09:12:24

yes but now you have to add the prefix every time. Homebrew can directly prefer arm tools instead.

borkdude09:12:45

Maybe this should be added to the docs?

👏 1
n2o09:12:35

Actually I am not quite sure if this is really a good solution. In my opinion, you should install the arm-version of homebrew, which then automatically selects the arm-based tools instead.

robert-stuttaford09:12:08

thanks n2o, still in the first hours with the new machine 😅 you've given me enough to dig with, thanks!

n2o09:12:10

I struggled several hours with it, because I do not want to specify the architecture on every installation 😅 A re-installation fixed it in the end. So that homebrew now prefers arm recipes over x64.

mknoszlig10:12:45

TIL: when running lein test on the clj-kondo repo the user-level (`~/.config..` ) config is applied. depending on the settings, this will cause tests that assume default behaviour to fail. what do you guys do to avoid this?

borkdude10:12:27

@maximilian you can try to add a metadata value on :config-paths ^:replace [] in the default test config, this will make it ignore the home dir

borkdude10:12:45

not sure if this will make other tests fail :)

mknoszlig12:12:42

well, it does make the test fail that’s supposed to check whether the config in $HOME gets picked up, but that’s a good thing i guess 🙂

mknoszlig12:12:43

i’m wondering … what’s your setup for running tests locally without having to comment your personal settings?

borkdude12:12:13

I guess I don't have conflicting settings with the tests

borkdude12:12:46

This is my personal config:

{:linters {:unsorted-required-namespaces {:level :warning}
           :used-underscored-binding {:level :warning}
           #_#_:shadowed-var {:level :off #_warning
                          :include [name]
                          :suggest {name nom}}}}

borkdude12:12:31

I'm fine with making the above change and tweaking the default config for the home test

borkdude12:12:39

what would be an example of a conflicting config though?

borkdude12:12:02

as in: can you give me one so I can experience this problem myself?

mknoszlig12:12:31

minimal example:

{:linters {:unused-binding {:exclude-destructured-keys-in-fn-args true
                            :exclude-defmulti-args true
                            }}}

mknoszlig12:12:47

fails on lein test :only clj-kondo.bindings-test/unused-binding-test

borkdude12:12:00

ok, but then we can just add the reverse config in the default config for the tests?

mknoszlig12:12:16

either that or in the test cases

borkdude12:12:26

I'd say in the default config

borkdude12:12:33

and then override that again in specific tests

mknoszlig12:12:40

yeah, that’s better, you’re right

mknoszlig12:12:40

one thing we’re doing with this approach however is duplicating default config into the test defaults.

borkdude12:12:42

there is one default config in test-utils

borkdude12:12:52

that's the only place where it should be updated I think

mknoszlig12:12:24

ok… glad to do a PR on this, but it would make sense to set defaults for all existing options, right? the tests that failed now were just due to my specific configuration …

borkdude12:12:34

Ok yes, I'm fine with doing this incrementally too. Whatever makes your life easier

borkdude12:12:09

Perhaps explicitly merging over the default kondo config will work

serioga10:12:35

I'm trying the new :docstring-no-summary linter. Why is the following positive?

(defn from-string
  "Initialize UUID from string representation.
   Accept only zero-padded representation."
  [_])
dev\clj_kondo_docstring.clj:4:3: warning: First line of the docstring should be a capitalized sentence ending with punctuation.

serioga10:12:26

looks like it does not work for multiline docstrings...

borkdude10:12:07

I'll make a fix

❤️ 1
serioga10:12:22

Should I file an issue?

borkdude10:12:13

I also found another issue, I messed up one keyword

borkdude10:12:30

so if you enable :docstring-no-summary you have also enabled :docstring-leading-trailing-whitespace now ;)

serioga11:12:01

all these linters look like options for the :missing-docstring :thinking_face:

borkdude11:12:10

don't understand

serioga11:12:39

sorry, my bad again 😊 forget it

serioga11:12:22

@borkdude how can I test clj-kondo from master? Usually I just install it by scoop.

borkdude11:12:16

you can also wait for the build to finish and download the windows binary from appveyor

joost-diepenmaat14:12:41

@serioga I think I messed up the regex for that docstring linter.

🙂 1
mknoszlig14:12:16

any test.check users out there? please take a look at https://github.com/clj-kondo/config/issues/16 - if you have linting needs beyond what is already in the ticket feel free to leave a comment 🙂 edit: updated link

borkdude14:12:28

@maximilian Are you aware that you can write hooks for any library / custom macros?

mknoszlig14:12:57

yes, but how would they be shared? the standard way would be with the library, correct?

borkdude14:12:33

Yes, and if the lib isn't accepting the contribution then via https://github.com/clj-kondo/config

borkdude14:12:59

I want to figure out how you can include external configs as dependencies so you will still get them via the same mechanism

borkdude14:12:13

without getting them all from the config library, just the ones you need

borkdude14:12:28

so perhaps we can make up something around that too

mknoszlig14:12:15

that would be great of course

borkdude14:12:09

We have done something similar for https://github.com/clj-easy/graal-config But there we hacked this by using :deps/root so we can put everything in one repository

borkdude14:12:40

But perhaps we should just make libraries under the clj-kondo organization that we can deploy to clojars, so leiningen users will get them as well

borkdude14:12:08

so each library would contain just the config and you can include that library in your deps

borkdude14:12:32

io.github.clj-kondo/config-clojure.test.check {:mvn/version "1.0.0"}

mknoszlig14:12:37

hmm… how would that work if you use eg the homebrew version?

borkdude15:12:25

yes, you would add this to your deps.edn / project.clj and then lint your project as you would always do. clojure-lsp does this automatically

mknoszlig15:12:59

ah, of course %)

borkdude15:12:05

@maximilian I guess we can just host these libs in one repository so we can use the same :deps/root trick as with graal configs and have an additional project.clj / build.clj in there for mvn deploys

borkdude15:12:29

that'll probably just mean reorganizing the clj-kondo/config project a bit

mknoszlig15:12:46

ok, so my issue should move there i guess 🙂

borkdude15:12:14

yeah, I'd rather see a user-space thing first, eventually we can move it as a built-in but I'd rather not take on more libraries in clj-kondo as built-ins unless they are part of clojure/clojure

mknoszlig15:12:19

makes sense. seeing compojure in impl/analyzer/ gave me the wrong idea i guess 🙂 - but that’s probably been there since before there was clj-kondo/config

borkdude15:12:07

yes, that's been there from the early days, just like the Schema stuff

borkdude15:12:54

we can eventually move "popular" configs back into kondo, but it's more flexible to have them outside first since they can be updated separately

👍 1