Fork me on GitHub
#clj-kondo
<
2022-09-01
>
Noah Bogart13:09:37

How interested would you be in auto-generated docs and default configuration for clj-kondo? I know you're not interested in splitting out the implementation of the rules, but I'm thinking that we could move the config info and documentation into individual files and then merge them as part of the build or release pipeline

borkdude14:09:21

Could be worth a try, but could also be a lot of work ;)

Noah Bogart14:09:41

haha yes it would, which is why I'm asking before doing anything dumb 😇

Noah Bogart14:09:00

I'll give it a go, see how fruitful/fruitless it is, and let you know!

Noah Bogart04:09:06

I've opened a draft PR to demonstrate colocating linter config and defaults, and their documentation: https://github.com/clj-kondo/clj-kondo/pull/1805. I'd like to get feedback from other clj-kondo hackers and devs as well as @borkdude himself.

Noah Bogart04:09:35

I went with a single file instead of multiple files, for ease of initial implementation, but splitting them would be quite easy.

lread12:09:33

Wow, that looks like a real valuable contribution @UEENNMX0T!

❤️ 1
Noah Bogart13:09:16

I think moving to multiple files is probably best, but then we'd want to bring in parse-edn-metadata-headers from markdown-clj or write our own so we could have edn metadata at the top and raw text examples below:

{:name "Cond-else"
  :keyword :cond-else
  :default-level :warning
  :description "Prefer `:else` as `cond` default branch."}

;;BAD
(cond
  (odd? (rand-int 10)) :foo
  :default :bar)

;; GOOD
(cond
  (odd? (rand-int 10)) :foo
  :else :bar)

Noah Bogart16:09:04

Sure, the wording can be changed. I just mean to have multiple examples for what is and is not allowed by a given rule. Here's an example from eslint, the de facto javascript linting engine: https://eslint.org/docs/latest/rules/for-direction

Noah Bogart16:09:23

> Examples of incorrect code for this rule:

Noah Bogart16:09:40

I’m hoping that by having more space for examples, we could list the alternatives and possibilities for each configuration, so the goal isn't to say definitively “X is good, Y is bad”, but “with linter L configured like this, X is allowed, Y is disallowed”

Noah Bogart16:09:19

Thanks for the link, I think @U05254DQM is right to avoid “good”/“bad” language

lread19:09:33

Funnily enough, it was probably bozhidar who wrote both the clojure and rubocop text. simple_smile Not suggesting there is a good or bad wording (see what I did there? :drum_with_drumsticks:). Just remembering some folks thoughtfully talking about it. Perhaps “Lint issue:” vs “Lint free” or something like that?

Noah Bogart19:09:33

Yeah, I’m not super worried about the specific wording lol. I just want to reduce friction for writing linter documentation, make it slightly easier and fix things like “linter X doesn’t have docs on accident”

borkdude20:09:20

• Problem statement. What I'm missing in this PR is a good problem statement: why are we making a big change, to solve what problem again? Big changes come with risk, so they should be justified • Performance. I haven't looked deeply yet at this PR but one thing I'm always concerned about is performance regressions or changes that affect startup time. Writing a literal map in the impl/config namespace is likely going to be faster than having to re-construct it from a string that has to be parsed from somewhere else on the classpath. And moving this stuff elsewhere also would break the clj-kondo pod (binary) so that would have to be fixed too (can be done, but something to remember). A few thoughts ◦ measure the time difference between loading the current clj-kondo and the clj-kondo with this PR. Also mind the laziness (I think I saw a map somewhere in the reconstructed version of the map): laziness often gives a skewed performance benchmark. So maybe replace those lazy things with mapv etc. ◦ measure how much time it takes to parse and reconstruct the map with a (time ..) expression ◦ Consider leaving the old map where it is and use that as the source of truth, while adding things to it from other places when generating docs ◦ Adding things to the "source of truth" map possible makes it more expensive to merge and merging configs is a thing that happens a lot. So not adding things to the old map when not generating documentation is preferred.

👍 1
Noah Bogart20:09:44

Thanks for the thoughts, Michiel, I'll write some stuff up and see what changes

borkdude14:09:23

So, I think your approach has benefits, but I also would like it if there would be no impact on the current way of doing things: minimal map literal (merge and loading time). Would it be possible to build the documentation stuff as another map + a deep-merge with the config in impl/config?

👍 1
Noah Bogart15:09:49

with the .edn file, on my 2018 mac book pro, it took roughly 2ms to load, read-string the edn file, and perform the transformation. moving it to a clj file makes it cost "nothing" to load, and the transformation step is roughly 0.05 ms.

borkdude15:09:08

The .edn file would also require a change to the binary, etc

borkdude15:09:26

or maybe not since it's a compile time constant...

borkdude15:09:12

the current state is that it's in a .clj file right?

borkdude15:09:26

seems good enough then. btw, you can get a tiny bit of perf win if you don't use :keys but just use (:foo ...)

borkdude15:09:46

but since this is not on a hot path, it doesn't matter

borkdude15:09:28

As a last step, can you update the dev.md docs to indicate what people should do when they want to add docs?

borkdude15:09:42

and/or place some hints in the code if people are going to look for this

Noah Bogart15:09:11

i'm also trying to figure out exactly how to write out what I want/hope to achieve here, which I know is a bad sign for merging, but it's more that it's harder to write about "dev experience" than "user experience". roughly, the process for writing a lint involves touching a lot of places: the specific part of the code that performs the check, the tests, the default config map, and the docs. The code and tests are to be expected, but the default config map and the docs are places where it's easy to make mistakes. For example, the docs disagree with the default config map on the default warning level for one of the lints (i didn't note which one, just used the one in the code). Both the config map and the docs are not in alphabetical order. The docs are missing some lints. Etc. This raises the burden of writing and maintaining the lints because all of these things have to be coordinated, and there's no automated way to know if they're in sync.

Noah Bogart15:09:42

Yes, right now I have it in a .clj file. lol sorry for writing all that out and missing your other messages

Noah Bogart15:09:17

I think it's a little harder to use as a .clj file, but that's mostly due to how we don't have multi-line strings in Clojure.

borkdude15:09:06

ah I see. well, put it in a .edn file then. 2ms is no biggie

👍 1
borkdude09:09:56

Btw, I didn't get the ".clj has not multiline strings" comment, how is that different from .edn?

Noah Bogart12:09:25

It’s not, I just found it harder to get indentation correct with additional indentation from the def form and quoting. Purely a personal preference.

borkdude13:09:24

You know what. Maybe just a script that checks if all linters have documentation should be enough. And alphabetically sorting the entries (this is not a must for me personally). I prefer writing .md than in .edn.

👍 1
borkdude13:09:49

This is where "start with a problem statements" comes in. This PR starts with a solution without a problem

borkdude13:09:10

If the problem is: you might miss writing docs for some linters, there should be various solutions and then a PR

Noah Bogart14:09:05

If that suits you better then I can do that. I think the problem is bigger than "missing docs for some linters", but like I said earlier, it's much harder to quantify "poor dev ex" so I understand. I agree that writing .md is much nicer than writing .edn and it's something I'm frustrated by in this attempt. I'll close this PR and open one focused on adding missing docs and sorting the documentation.

🙏 1
borkdude14:09:42

Thanks. I appreciate it

Noah Bogart14:09:19

I'm gonna keep poking at this because I think it's worthwhile, but I'll wait until I have a much better solution. Thanks for all the feedback, I really appreciate your patience and continued help

lread14:09:32

@UEENNMX0T I find working out what problem I am trying to solve in a GitHub issue (or a GitHub discussion) often helps me out. I know I appreciate this kind of thing too on projects I maintain.

👍 1
Noah Bogart14:09:32

hah Yeah, I generally do that first but jumped the gun here. Oops!

lread14:09:50

Yeah, there have been rumours of me running with scissors too!

😂 1