Hey @borkdude I would like to hear your opinion on this clojure-lsp "bug" after the keyword analysis
@ericdallo Storing the name of the keyword as a string would help here. Alternatively clj-kondo could report the keyword as invalid and not output it in the analysis.
EDN should not be able to read symbols starting with a number. The spec already says these keywords aren't valid.
Yeah, I see, not reporting probably would be better
Since we are parsing a giant analysis from the sqlite and have no way to know which analysis is wrong
So what does that PR merged do?
but if clj-kondo offered the name as a string, then it would be ok I think?
Yes, we 'd persist as a string
I reverted the merge, so let's just look at the problem from first principle
Just tested and a string value would be parsed correctly from db so probably the best approach for keywords buckets (or all buckets to keep the standard?)
We're already exposing the other buckets as documented features. I kept the keyword analysis undocumented to give it a trial run first
Maybe changing all the names to strings won't be a major break though, since (keyword ..)
etc take symbols and strings?
Yes, it takes, looks valid to me
Created thsi to track on clojure-lsp side: https://github.com/clojure-lsp/clojure-lsp/issues/305
Something like that works great for me
(edn/read-string "{:name :2s}")
This would then be :name "2s"
oh, yeah, you are right
we'd need only the paces that use :name, to parse the string on clojure-lsp, not hard though
how are you using :name
rigth now?
One place builds the full keyword from a analysis (`:foo/bar` from :alias
and :name
) and all the others use to compare one keyword analysis with another keyword analysis
so, looks changeable
yeah, we can change the keyword analysis surely, because no-one else is using that. but I meant the :name fields in the other analysis
oh got it, let me check
we can just change it for the keyword analysis though, PR welcome
can do it now
yeah, most places we get the string with (name ...)
and others we compare with other fields. like :to
:from
:alias
etc, I think would work, but some refacotr/check in all features
Maybe strings would have been better to start with. For people using JSON they won't notice the difference but I think there's clojure tooling which might break if we changed that
Ok, I can do it soon for keywords
yeah, for sure, for other analysis we need to be safe
Just for reference: https://github.com/clj-kondo/clj-kondo/pull/1162 will make the proper changes on clojure-lsp. thanks
merged
Tested with clojure-lsp and it worked great!
yay!
Bug: As of now, if there is anyone with a keyword on the project/deps that starts with a number, it'll will not be able to read from cache(sqlite) when starting the project.
So, I noticed that every time I start clojure-lsp on clojure-lsp
project, it's not being able to read the edn persisted on sqlite, because there is a simple keyword analysis with name :2s
from https://github.com/ptaoussanis/encore/blob/645dc120380ab75648cd449c17f24b381055977d/src/taoensso/encore.cljc#L2268.
We save the analysis with transforming it to string with pr-string
and when load it, we read with edn/read-string
.
When parsing the edn {:name 2s}
, clojrue just throw a NumberFormatException
, I tested it on REPL and indeed when you type '2s
it just blow up.
So i'm not sure what is the issue here, should clojure.data.edn/read-string
be able to parse a symbol that starts with a number? otherwise how should we proceed? should clj-kondo save the :name
of a keywords bucket with the :
?