Clojurians Log v2

https://github.com/clojure-lsp/clojure-lsp/issues/1876

yeah that's weird 😂 we rely on clj-kondo findings for that, maybe some bug in the api? any thoughts @borkdude?

this might be a leftover from the array notation thingie... let me check

are you using the newest clj-kondo?

v2024.08.29

I guess lsp is behind

$ clojure-lsp --version
clojure-lsp 2024.08.05-18.16.00
clj-kondo 2024.08.01

"is there a way to run the newest clj-kondo with lsp"? yes

yeah, i've set it up in the past, just don't have it configured right now

i don't mind, i run both clj-kondo and clojure-lsp in my CI, just noticed in my editor this wasn't getting flagged

Just pushed this bump to clojure-lsp, should be a #clojure-lsp-builds nightly build soon

@nbtheduke any reason why not use in your CI clojure-lsp diagnostics ?

we are using for all Nubank services and it works great

(it adds the option to have clj-depend and other custom linters as well)

btw @ericdallo can you bump to the latest master commit if you haven't? I fixed a post-release bug in there

ah ok

Done

AFAICS the current implementation marks all the defs in all the test namespaces as unused

Because only test namespaces are referring to test defined functions and defs

So I believe there the test references should not be excluded

gotcha, I was able to create a test scenario for this 😉 I'll have a look at it tomorrow, thanks for reporting it!

reviewplease https://github.com/clojure-lsp/clojure-lsp/pull/2009

Merged, @ikaraszi could you test a nightly build later #clojure-lsp-builds and confirm it fixed for you?

@ikaraszi could you try https://github.com/clojure-lsp/clojure-lsp/pull/1992 branch?

Thank you, I’ll try it. I didn’t have performance issues, so I can only test the functionality

So the string/includes? vs string/ends_with? was the culprit?

AFAICS this wont support cljs, right?

@andreribeirocamargo may have it, I don't remember exactly how we repro the problem when we found the regression, we should have commented on the issue

Hello, it was taking longer on some big internal Nubank projects. I'll have a look at this again and let's see how it goes.

> So the string/includes? vs string/ends_with? was the culprit? no, the first algorithm is more resource-intensive than the new one. > AFAICS this wont support cljs, right? yes, it won't. is this an issue for you?

> yes, it won’t. is this an issue for you? That is not an issue for me, just thought it is worth to mention

Anyhow, I’ll give it a try

reviewplease https://github.com/clojure-lsp/clojure-lsp/pull/1994

@andreribeirocamargo remember to update all-available-settings.edn

What should I change there? There's no :clojure-lsp/unused-public-var linter configuration there 🤔

hum, true, that only applies for clojure-lsp settings, nvm

I wonder if we should move this setting to clojure-lsp settings itself... as there are other features that need to know if file is a test, the code lens for example to know if show a reference as a test or not

I like the idea, let's think more about this

I've made some changes to the PR in order to implement this vision

I can modify the settings and set the :source-paths to #{"src"}, but I was thinking a more convenient way to do this

https://clojure-lsp.io/settings/#clojure-lspunused-public-var You can use :exclude-when-defined-by or maybe :exclude

the main issue is: there is no canonical way to know if a function is using in a test, we can only infer that

Yes, I understand that part. So I could do:

:exclude-regex #{"my-ns/.*-test$"}

Yeah, I guess Maybe we could have a option for that since we already infer those for code lens

That would be great

Please create a issue so I can prioritize and discuss further more

Thanks, doing it now

https://github.com/clojure-lsp/clojure-lsp/issues/1878

But this should work, right?

:clojure-lsp/unused-public-var {:level :warning
                                :exclude #{dev}
                                :exclude-regex #{".*?-test$"}}

Not sure about the regex, but looks correct

I added the ? to turn off the greediness but I could not get it working

I didn't get that ? , shouldn't you use ".*-test$" to exclude everything that ends with -test ?

I tried that as well, didn’t work.

But I can try it again tomorrow

It works like a charm! Thank you very much! 🙏

(re-matches #".*-test$" "foo.bar.something-test")
;; => "foo.bar.something-test"
user> (re-matches #".*?-test$" "foo.bar.something-test")
;; => "foo.bar.something-test"

both of them are matching, but I’ll try to modify my config

This is what I have in my .clj-kondo/config.edn:

:clojure-lsp/unused-public-var {:level :warning
                                :exclude #{dev}
                                :exclude-regex #{"metric.*-test.*"}}

Okay, I believe I misunderstood the intention of the :exclude, it excludes the warnings in these namespaces, and not as I thought excludes the namespaces from the callers list.

Then this is definitely a missing feature, I cannot have a workaround with a regex

Ah you are right, it's a different concept, maybe the flag should be: :disconsider-test-usages

or maybe we can flip the logic by saying :include-tests false and per default it is true?

but still it is orthogonal to the other exclude, so probably not the best choice

I don't wanna change the default right now as there are lots of clients using this linter, but in the future it may be a good idea

naming is hard, gosh

Yeah haha

:ignore-tests true?

or similar to your suggestion: :disregard-test-usages true

“disconsider” is a rarely used word, probably that’s why it’s sounds strange to me

@ericdallo are you planning to bring back this feature?

sorry, what's the problem again?

You know that you implemented an exclusion solution so references only by test namespaces did not show as usages

And with this feature somebody could remove unused functions that only called from tests

You needed to remove this feature in a later release because of a performance regression

Ah right, I lost your comment about reopening the issue, we still need to check how to implement it without losing performance

Can I help in any ways?

well, you could try to re-implement https://github.com/clojure-lsp/clojure-lsp/commit/0f81895571bb1fb6fe77ca5746241942b17ab9d4 but try to improve the performance, unfortunately I don't have the answer on how to do it

Thanks! I’ll check. I could only see this part which could be problematic: https://github.com/clojure-lsp/clojure-lsp/commit/0f81895571bb1fb6fe77ca5746241942b17ab9d4#diff-048949e9115a6b272d3aad1438dbd11269358c2198114ec28f23079cc3ecf422R268-R272

yes, maybe you could do a perf flamegraph to understand what is taking so long

Yes, I’ll have time today. Thank you very much guys!

Something is still off with:

clojure-lsp 2025.04.01-20.27.48-nightly
clj-kondo 2025.02.21-SNAPSHOT

My take is: the namespace metric.test-fixtures doesn't follow https://guide.clojure.style/#test-ns-naming of having a suffix -test, so Clojure-LSP is considering that code as application code. I'd rather suggest you to change :test-locations-regex in .lsp/config.edn from #{"_test\.clj[a-z]?$"} to #{"/test/metric/"} . This change will make Clojure-LSP to consider all files with "/test/metric/" in their path as test code. Ideally, you'd tailor the :test-locations-regex to reflect where the tests live (it's a Clojure set, you can have multiple regexes). Give it a try on this and let me know how it goes 😉

Because that is a helper namespace that does not contain tests just functions that can be used in tests, but it is under the test folder. I’ll test the :test-locations-regex as you suggested.

I believe it works! 🎉

Awesome! Clojure doesn't differentiate application from test code. That's why we have this convention of appending -test to test namespaces and functions. This also prevents clashing tests with application code. So Clojure-LSP relies on :test-locations-regex to understand what is test and what is not.

do you have a repro?