clj-kondo

https://github.com/clj-kondo/clj-kondo
ericdallo 2021-02-07T04:18:21.102Z

Hey @borkdude I would like to hear your opinion on this clojure-lsp "bug" after the keyword analysis

borkdude 2021-02-07T09:05:16.102800Z

@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.

borkdude 2021-02-07T09:06:12.103Z

EDN should not be able to read symbols starting with a number. The spec already says these keywords aren't valid.

borkdude 2021-02-07T09:06:29.103200Z

Also see https://github.com/clj-kondo/clj-kondo/issues/1160

ericdallo 2021-02-07T12:22:20.103600Z

Yeah, I see, not reporting probably would be better

ericdallo 2021-02-07T12:22:50.103800Z

Since we are parsing a giant analysis from the sqlite and have no way to know which analysis is wrong

ericdallo 2021-02-07T12:23:06.104Z

So what does that PR merged do?

borkdude 2021-02-07T12:23:11.104200Z

but if clj-kondo offered the name as a string, then it would be ok I think?

ericdallo 2021-02-07T12:23:41.104500Z

Yes, we 'd persist as a string

borkdude 2021-02-07T12:23:48.104700Z

I reverted the merge, so let's just look at the problem from first principle

👍 1
ericdallo 2021-02-07T12:54:00.105Z

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?)

borkdude 2021-02-07T12:55:42.105200Z

We're already exposing the other buckets as documented features. I kept the keyword analysis undocumented to give it a trial run first

borkdude 2021-02-07T12:56:22.105400Z

Maybe changing all the names to strings won't be a major break though, since (keyword ..) etc take symbols and strings?

ericdallo 2021-02-07T12:58:30.105600Z

Yes, it takes, looks valid to me

ericdallo 2021-02-07T12:58:50.105800Z

Created thsi to track on clojure-lsp side: https://github.com/clojure-lsp/clojure-lsp/issues/305

ericdallo 2021-02-07T13:01:59.106100Z

Something like that works great for me

(edn/read-string "{:name :2s}")

borkdude 2021-02-07T13:06:39.106300Z

This would then be :name "2s"

ericdallo 2021-02-07T13:07:50.106500Z

oh, yeah, you are right

ericdallo 2021-02-07T13:08:14.106700Z

we'd need only the paces that use :name, to parse the string on clojure-lsp, not hard though

borkdude 2021-02-07T13:10:31.106900Z

how are you using :name rigth now?

ericdallo 2021-02-07T13:42:54.107100Z

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

ericdallo 2021-02-07T13:43:01.107300Z

so, looks changeable

borkdude 2021-02-07T13:44:22.107500Z

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

ericdallo 2021-02-07T13:46:06.107700Z

oh got it, let me check

borkdude 2021-02-07T13:46:29.107900Z

we can just change it for the keyword analysis though, PR welcome

borkdude 2021-02-07T13:46:37.108100Z

can do it now

ericdallo 2021-02-07T13:49:47.108300Z

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

borkdude 2021-02-07T13:50:39.108500Z

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

ericdallo 2021-02-07T13:50:41.108700Z

Ok, I can do it soon for keywords

ericdallo 2021-02-07T13:51:07.108900Z

yeah, for sure, for other analysis we need to be safe

ericdallo 2021-02-07T14:10:45.109300Z

Just for reference: https://github.com/clj-kondo/clj-kondo/pull/1162 will make the proper changes on clojure-lsp. thanks

borkdude 2021-02-07T14:10:58.109600Z

merged

👍 1
ericdallo 2021-02-07T14:50:23.109900Z

Tested with clojure-lsp and it worked great!

borkdude 2021-02-07T15:00:42.110100Z

yay!

ericdallo 2021-02-07T04:25:59.102200Z

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 : ?