What is the correct way to handle this warning from cl-kondo?
(defmulti rte-match
(fn [rte _items & {:keys [promise-disjoint
hot-spot]}]
(dispatch rte 'rte-match)))
(defmethod rte-match :sequential
[pattern items & {:keys [promise-disjoint
hot-spot]}]
(rte-match (rte-compile pattern) items :promise-disjoint true :hot-spot hot-spot))
It warns that promise-disjoint
and hot-spot
are not used in the defmulti
, and that promise-disjoint
is not used in the first defmethod. However, these keys ARE used in other defmethods.
I'm not sure whether this is a clojure question or a clj-kondo question actuallyshouldn't the defmulti declare all the keys which methods are allowed to consume?
no, they don’t have to
"have to"? OK I agree it's not strictly necessary. but it seems to me that the defmulti should serve as documentation to applications which want to add methods. right?
you can write it like this
(defmulti rte-match
(fn [rte _items & _]
(dispatch rte 'rte-match)))
(defmethod rte-match :sequential
[pattern items & {:keys [hot-spot]}]
(rte-match (rte-compile pattern) items :promise-disjoint true :hot-spot hot-spot))
defmulti
is a strange beast (imho) in terms of documentation
since dispatch function can be detached from declaration of defmulti
If I do that, then where is the documentation for the callers of the methods to use, and where is the documentation for implementors of methods to use?
(defmulti foo some-dispatch-fn)
in a docstring after the name of defmulti
(defmulti foo "docstring" some-dispatch-fn)
good idea.
on the other hand, clj-kondo seems to have the feature that normal parameters which are not used can be prefixed with _ to disable the warning.
(defn foo [a b _c d] ...)
clj-kondo will not warn that _c is unused.
Is there something similar for key arguments?right
by convention arguments beginning with _
(including single character _
) are “might not be used”
but you still can use them in function’s body
(fn foo [_x _y] (+ _x _y))
I see that clj-kondo fails to warn if I use an _ variable. That's unfortunate.
?
in Common Lisp you can mark them as the three different cases.
I.e., in CL i can declare the variable as either unused or maybe-unused
as I mention this is a convention not a rule in clojure
and thus if I mark something as unused, but still use it i get a welcomed warning.
_
or _x
are just normal symbols that can be used as a names of vars
(defn foo [_] (inc _))
indeed, but kondo is applying some conventions to the names to aid the programmer in static analysis, right, clojure doesnt care.
exactly, and you always can switch this warnings on and off
but maybe I’m wrong, better to ask @borkdude if it is possible to turn on warning about “unused vars” beeing used
I’m afk today until the evening. Check config.md for documentation about config.
I will open an issue because there is no option (as I can see in config.md) to turn on such warnings
This has already been suggested in one of the issues. Can’t really Google it on my phone :-)
my google fu failed me) will check one more time
found it
ah that was my issue from long ago. I forgot about that. but now it rings a bell.
I looked for a bug all day. The bug occurred when I moved a bunch of functions around, basically refactoring name spaces. The problem was that when I run my tests from the lein command line they fail. But when I run them interactively the succeeded.
The problem finally turned out to be I was defining the method in two different files.
The defmulti is in a file whose namespace is genus
and is abbreviated gns
every where which requires it. The intent is that other files can add methods to gns/-disjoint?
.
Unfortunately two different files added the method
(defmethod gns/-disjoint :rte ...)
with two different implementations.
QUESTION: Shouldn't clj-kondo warn me about this?on the surface that feels like a nice linter
it is perfectly legal to redefine methods. otherwise debugging would be more difficult. But static analysis should warn.
I'll file an issue.
everyone should rush there and give it a thumbs up. 👍:skin-tone-2:
I think it doesn’t really matter where defmethod is redefining same dispatch value right now clj-kondo will not complain even about this
(defmulti foo identity)
(defmethod foo :x [_] :x)
(defmethod foo :x [_] :y)
all in the same namespaceshould it though is the question really
sure it is
?
that code above feels smelly and is more likely than not the cause of a bad copy/paste
don’t mind right hand side of my examples ) the important part is on the left and it is not necessary bad copy/paste but could be some leftovers after refactoring or intensive REPL usage
(defmulti foo identity)
(defmethod foo :x [_] :x)
(defmethod foo :x [_] :y)
this example right now looks ok for clj-kondo but might be considered as perfect place for warning about defining different implementations for the same dispatch value of multi methodyeah artifacts of refactoring/updates/etc
ok, makes sense
makes sense
Agree that it doesn't matter where the method is defined. My comment was simply that it might be defined as a naked name in one place (defmethod foo :x ...)
and as a decorated name elsewhere (defmethod mynamespace/foo :x ...)
and that still deserved a complaint from kondo. in my opinion.
@borkdude do you want me to create an additional circleci path to create the with-profiles standalone.jar?
@b Just commented. I think it's better if you removed the native-image
profile for the uberjar creation, since that needs JDK11 (it pulls in JDK11-specific deps)
But an uberjar created with Java 8 should be compileable with Java 11, so I think then it should work
I must be blind, but there is no mention of java8 in any .circleci/config.yml
it downloads graalvm11 only
circleci/clojure:lein-2.8.1
has java 8 by defaultthat's quite unfortunate because I had to use both profiles for it to work with native-image for me
I used the clojure:lein-2.8.1 image on my own system to generate the standalone jar and then transfer it to my real host
weird that I didn't get that error, that's why I wrote about the caching of .m2 as a potential cause
but I also build everything after I already had prepended the graalvm directory to my PATH
@b It might be better if the native-image step on CircleCI just posted the uberjar as an artifact as well. This way you are guaranteed to use the same one
So instead of in the JVM step
so sidestepping the normal jobs and creating an additional single job that does everything needed to create the standalone jar?
could do, but could also do it in the linux job
@b Right now we have these files: https://app.circleci.com/pipelines/github/borkdude/clj-kondo/3030/workflows/1c2aa51c-ca4c-4c2b-a95d-d2e924909f54/jobs/11540/artifacts We could simply add the uberjar to it
And then I will post that in the github releases in the future
oh wow, I just tried to reproduce it and I found why I thought it worked in my setup: I tested the circleci steps in circleci/clojure:lein-2.9.1 instead of lein-2.8.1
and that one has java11 right
lein-2.9.1 comes with java11 yeah
I'm using up all your circleci credits with my fails 😮
"$VERSION" -eq "linux"
should probably be:
"$CLJ_KONDO_PLATFORM" -eq "linux"
(not sure about -eq
, I always use =
for strings in bash)environment:
LEIN_ROOT: "true"
GRAALVM_HOME: /home/circleci/graalvm-ce-java11-20.2.0
CLJ_KONDO_PLATFORM: linux # used in release script
CLJ_KONDO_TEST_ENV: native
so you were close
fair enough and you're probably also right about the bash thing, -eq is for checking return code results 😉
but I think it might not hurt to copy the uberjar if it's not on linux
Technically -eq is for checking integers, = is for strings. In bash.
do you manually download the artifacts then? I didn't want to overwrite anything in case you automatically merged all the artefacts
yeah, I do it manually
cp: cannot stat 'target/clj-kondo--standalone.jar': No such file or directory
adding: clj-kondo (deflated 71%)
We might want to add set -e
to the script
okay
set -eo pipefail
man I don't know what I'm doing right now, sorry for not paying attention.
laptop on couch is not great for concentration
success!
seems to work now right? https://app.circleci.com/pipelines/github/borkdude/clj-kondo/3034/workflows/f45a9937-a0f6-46a2-ba8d-fa517458e928/jobs/11556/artifacts
as a last step, I think we can now remove the uberjar from the jvm tests
I'll test the resulting standalone jar (but it looks good due to the size) and then remove the jvm uberjar creation and rewrite the commit message
:thumbsup:
looks good, but while we're at it. should we save the earth and also zip the jar? a simple zip invocation reduces the file size by more than 50%
a jar is already a zip file
yeah I know
but I don't know how to tweak the settings to have it user better compression
while zip just reduces the filesize to 45%
I would say, let's leave it at this for now
:thumbsup:
Merged the first batch of checks for deps.edn to master: https://github.com/borkdude/clj-kondo/blob/a9df3965716025e93764d36100314edca30e5cee/test/clj_kondo/deps_edn_test.clj