Fork me on GitHub
#clj-kondo
<
2019-08-20
>
gordon09:08:18

FYI I haven't forgotten about testing https://github.com/borkdude/clj-kondo/issues/422 I'm seeing some filesystem permission errors writing the lockfile when using the cache with that binary, which I'm willing to blame on my recent upgrade to macOS Mojave until I can prove otherwise.

borkdude09:08:12

can you try to throw away the .cache directory?

gordon09:08:27

I've done all the things that were obvious to me, checked permissions on binary + dir, removed the dir (it can create the cache dir fine, just can't write the lockfile), checked for macOS extended attributes.

gordon10:08:03

Anyway, I'll keep poking at it and let you know when I have a result.

borkdude10:08:12

do you maybe have some anti-virus software running? I recently had an issue with this while compiling CLJS: it became terribly slow

gordon10:08:06

Ah, I do indeed, this is my work computer.

borkdude10:08:25

maybe clj-kondo is not able to write at all then due to that malware/virus program

gordon10:08:17

lein clj-kondo works fine which adds to my confusion. Anyway, I'll keep digging.

gordon10:08:47

This is an alias that invokes clj-kondo.main it turns out.

borkdude10:08:41

maybe java is whitelisted as a trusted process, but the binary clj-kondo is suspicious somehow?

borkdude10:08:57

guessing...

gordon10:08:20

Running without --cache works well enough, I don't see the recur argument mismatch any more :thumbsup:

Macroz10:08:51

flycheck-clj-kondo seems to be working in my Emacs but it's not respecting ns inline config, I guess it should work?

Macroz10:08:18

I'm trying to ignore some warnings but they just keep showing up

Macroz10:08:59

i.e. trying something like this

{:clj-kondo/config
   '{:linters {:redefined-var {:level :off}
               :refer-all {:level :off}}}}

Macroz10:08:33

btw it would be helpful to show the linter config key in the error tooltip

borkdude10:08:32

not all linters may work yet with the ns config. can you test with clj-kondo from master?

borkdude10:08:19

you can run with the jvm version to test

Macroz10:08:12

looks like you have different uses and corner cases well covered 🙂

Macroz11:08:58

hmm cloning the repo and running clj-kondo from there seems to give me the warnings

Macroz11:08:19

so which one should work with inline config?

borkdude11:08:41

do you mean that clj-kondo from master works as desired for you?

borkdude11:08:01

if so, I can give you the binary from master

borkdude11:08:21

and then after I release the next version, you can start using it from a normal package manager again

Macroz11:08:37

it doesn't

Macroz11:08:52

it gives me warnings though I tried to disable them in the ns inline config

Macroz11:08:02

like the scenario from the example screenshot

Macroz11:08:38

I got kondo nicely running in emacs which is good but the inline config to disable some warning sometime seemed like a good thing to get to work

Macroz11:08:04

I'm wondering can I somehow test that I didn't do a stupid mistake or contribute something if it doesn't work well yet

Macroz11:08:47

I don't have an open source repo here at the moment, testing on some internal code for now

Macroz11:08:00

at least I can confirm that I'm running the version from master

Macroz11:08:38

seems to work with --config '{:linters {:redefined-var {:level :off}}}'

Macroz11:08:53

so just the ns inline config doesn't get picked

Macroz11:08:57

I see you have a test like this

(is (empty? (lint! "(ns ^{:clj-kondo/config
                            '{:linters {:unused-namespace {:exclude [bar]}}}}
                          foo
                        (:require [bar :as b]))"))))

Macroz11:08:29

I can't get that to work

borkdude11:08:45

Please post an issue with the problem and repro, then I'll have a look

Macroz11:08:28

I get four errors if I run lein test

Macroz11:08:56

I wrote a failing test for my case and I can see if I can patch it

Macroz11:08:09

but just noticed that there are other errors too

borkdude11:08:33

without any changes you mean? that's weird, because on CI they pass?

borkdude11:08:51

could you try with clj -A:test too?

Macroz12:08:19

same result

FAIL in (extract-clojure-core-vars-test) (extract_var_info_test.clj:12)
expected: (contains? vars (quote future))
  actual: (not (contains? #{} future))

FAIL in (extract-clojure-core-vars-test) (extract_var_info_test.clj:13)
expected: (contains? vars (quote transduce))
  actual: (not (contains? #{} transduce))

FAIL in (extract-cljs-core-vars-test) (extract_var_info_test.clj:17)
expected: (contains? vars (quote clj->js))
  actual: (not (contains? #{*target* ns js*} clj->js))

FAIL in (extract-cljs-core-vars-test) (extract_var_info_test.clj:18)
expected: (contains? vars (quote transduce))
  actual: (not (contains? #{*target* ns js*} transduce))

Macroz12:08:40

maybe your circle build doesn't run tests?

borkdude12:08:15

let me try locally

Macroz12:08:46

I see now that I need to run script/test first, without that there are some files missing

borkdude12:08:57

that's right, also what is documented in the README

borkdude12:08:29

maybe those deps can be just used to the test deps

borkdude12:08:01

they are not really used as deps, but used for extracting information

Macroz12:08:49

that part of the README is so short that it's easy to miss

Macroz12:08:43

also kind of expected to see it in the build doc

borkdude12:08:33

afaik it's not needed for building

Macroz12:08:07

yes, but commonly development i.e. build, test, deploy is together

Macroz12:08:54

it's not a big deal though, just something bumped into

Macroz12:08:31

could be in a contributor doc as well, or PR/issue template

Macroz13:08:31

hmm it seems that you load the config "file" and use that for the filtering levels and don't use the ns local configs for filtering so it cannot possibly work

Macroz13:08:35

I have always had a need to disable some linting rules locally, and it makes sense to do that in code metadata, comments or so

Macroz13:08:07

I think the filtering of the results needs to be rewritten to work with the analyzer that actually resolves and propagates the local configs

borkdude13:08:24

what exactly do you want to disable using a ns local config?

Macroz14:08:04

for example I have some redefinitions of + - etc. in one file that reimplements some math concepts

borkdude14:08:25

then you should just use :refer-clojure :exclude [+ -]

Macroz14:08:26

or there is one function that does something more magic than the rest of the code ever needs to do

Macroz14:08:04

I still need the core + and - in the implementation of my + and -

borkdude14:08:19

you can refer to those as core/+

Macroz14:08:22

often I may also name something local key or value etc.

Macroz14:08:38

so I naturally tried using :level :off for those reporters

borkdude14:08:39

that's fine, kondo doesn't complain about that

Macroz14:08:12

my experience is more from Java and JavaScript linters and they usually support turning off a warning here or there

Macroz14:08:26

these are just the ones I noticed when opening a random file I was working on today

Macroz14:08:35

once you have more rules there will be more cases

borkdude14:08:22

(ns foo
  (:refer-clojure :exclude [+ -])
  (:require [clojure.core :as c]))

(defn + [a b]
  (c/+ a b))

(prn (+ 1 2))
This is how you should do it in Clojure. Not using exclude while redefining things is bad.

borkdude14:08:02

The linters that make sense to support as namespace local configs are supported, unless someone comes up with good examples of other linters that should be supported.

borkdude14:08:23

Feel free to post an issue with good examples and we'll make it work.

borkdude14:08:07

The reason why not everything is supported is that ns local config is a new feature and was only used for testing

borkdude14:08:22

clj-kondo has the philosophy that code should not be cluttered with tooling annotations, so where possible, use the global config

Macroz14:08:48

maybe you could just add a comment in the config section that local config does not work for every option

borkdude14:08:25

That is already documented. > Config takes precedence in the order of namespace, command line, .clj-kondo/config.edn. *Note that not all linters are currently supported in namespace local configuration.*

Macroz14:08:27

that is different to what I mean, :level is a linter option

borkdude14:08:48

yeah, it could be documented which linters do not work with ns local config

Macroz14:08:58

your specific example in tests uses ns local config but does not support level option

(is (empty? (lint! "(ns ^{:clj-kondo/config
                            '{:linters {:unused-namespace {:exclude [bar]}}}}
                          foo
                        (:require [bar :as b]))")))
vs. a hypothetical
(is (empty? (lint! "(ns ^{:clj-kondo/config
                            '{:debug true
                              :linters {:unused-namespace {:level :off}}}}
                          foo
                        (:require [bar :as b]))")))

Macroz14:08:45

since it's pretty clear level doesn't work then you could just mention that, I don't see a point trying to track code functionality completely in manually written documentation

Macroz14:08:55

tuning that quoted piece of doc would have been enough for me

Macroz14:08:12

I did notice it but thought that if the linter is supported then perhaps its options are

borkdude14:08:15

ah now I see what you mean. so you want a different level only for that namespace.. hmm

borkdude14:08:38

feel free to post an issue about that. I think that can work

borkdude14:08:12

I just hadn't considered that.

Macroz14:08:53

the problem in the current code is that the local configs don't get to the filter so it needs a slightly larger change than I can do myself at the moment but I'll do an issue tomorrow

borkdude14:08:00

I think the problem is that during output, the level from the global config. But you can override it I think, it's just ignored at the moment

borkdude14:08:19

so if we're luckily, it's just a minimal change. I'll look at it when I'll get to your issue

borkdude10:08:05

@gordonsyme_clojurians from the next version on, --cache will become the default so you will have to do --cache false, just as a heads up

👍 4
borkdude10:08:22

right now that option is interpreted as a directory, so that won't work right now

borkdude10:08:36

or maybe it will, not sure

gordon10:08:34

Thanks for the warning, I need to get to the bottom of this issue, but that's a problem for me to get resolved. Thanks for being so responsive 🙂