rewrite-clj

https://github.com/clj-commons/rewrite-clj
borkdude 2021-05-01T15:56:18.238300Z

@lee Is there a way to do this:

(defn- rightmost?
  [zloc]
  (nil? (ws/skip z/right* ws/whitespace? (z/right* zloc))))
without relying on rewrite-clj.zip.whitespace?

borkdude 2021-05-01T15:56:54.238500Z

(this is from cljstyle)

lread 2021-05-01T17:25:51.241700Z

Yeah, I think so. Both skip and whitespace? from rewrite-clj.zip.whitespace are exposed to rewrite-clj.zip. So the above could be rewritten as:

(defn- rightmost?
  [zloc]
  (nil? (z/skip z/right* z/whitespace? (z/right* zloc))))

ericdallo 2021-05-01T17:47:01.242100Z

About the vertical align form @lee 🧵

ericdallo 2021-05-01T17:49:09.242200Z

I already saw multiple preferences regarding that, people/projects/orgs that prefer to vertical align forms on everything like let/maps etcs and pople that dislike it on the other side. I think we should support for all those cases for a better UX. My preference is don't use the vertical align at all, but I agree that it works good in some places like the bb.edn tasks like @borkdude pointed in the other thread

lread 2021-05-01T17:50:41.242400Z

Yeah, vertical alignment is certainly subjective.

lread 2021-05-01T17:51:11.242600Z

I some cases, I find it helps my old eyeballs.

lread 2021-05-01T17:53:02.242800Z

I see some chatter over in cljfmt issues about performance problems. Would you prefer a look-see into that first?

lread 2021-05-01T17:54:35.243Z

Note to lurkers: we are chatting about adding vertical alignment support to cljfmt, not rewrite-clj.

lread 2021-05-01T17:55:35.243300Z

For alignment, I’ll go see what control cljstyle offers… maybe we can steal some ideas from them.

ericdallo 2021-05-01T18:00:45.243500Z

Yeah, I opened this issue about performance: https://github.com/weavejester/cljfmt/issues/226 this is something that really makes users disable format on clojure-lsp because of that performance issue 😕

lread 2021-05-01T18:06:43.243800Z

For alignment… might have missed it, but don’t see specific rules in cljstyle… not sure how it decides to vertically align…

lread 2021-05-01T18:09:39.244Z

For cljfmt perf… yeah I’m kind of curious. Your use case is clojure-lsp which is Clojure only, right? (not ClojureScript?).

ericdallo 2021-05-01T18:10:54.244200Z

yes, there is no specification on cljstyle, I think the "common" idea is to align vertically with the longest word

1👍
ericdallo 2021-05-01T18:11:08.244400Z

Yes, clojure only, but via native image on GraalVM 👀

ericdallo 2021-05-01T18:11:36.244600Z

but I noticed those performance issues on both, so I don't think graalvm is the issue

lread 2021-05-01T18:13:45.244800Z

Right… maybe I’ll start by seeing if I can reproduce some slowness on the JVM. And then maybe I’ll play with com.clojure-goes-fast/clj-async-profiler to see what might be the culprits.

ericdallo 2021-05-01T18:14:53.245Z

nice, I usually notice the performance issue with medium/big forms like I mentioned on the issue. LMK if need any help/testing

borkdude 2021-05-01T18:18:34.245300Z

Yeah, I figured. Thanks :)

lread 2021-05-01T18:22:24.245500Z

Rule of borkdude: if you don’t answer in three minutes, you can assume he’ll have figured it out. :simple_smile:

ericdallo 2021-05-01T19:36:57.245700Z

Found this on cljstyle: https://github.com/greglook/cljstyle/issues/47

ericdallo 2021-05-01T19:38:09.246Z

And also this 😅 https://github.com/weavejester/cljfmt/pull/77

lread 2021-05-01T20:21:36.246300Z

thanks for links!

lread 2021-05-01T20:21:58.246500Z

I’ll start a new thread for cljfmt perf so we don’t confuse ourselves too much!

1👍
lread 2021-05-01T20:22:20.247Z

Cljfmt performance @ericdallo 🧵

lread 2021-05-01T20:22:43.247100Z

I’ve made a first crack at reproducing the issue https://github.com/weavejester/cljfmt/issues/226#issuecomment-830688654

ericdallo 2021-05-01T20:25:57.247500Z

Thanks! I replied it and I'm starting to think that the performance issue is not 100% deterministic, I already faced it multiple times but never found the correct repro

lread 2021-05-01T20:26:18.247800Z

Related to alignment might be https://github.com/weavejester/cljfmt/issues/49 which suggests that cljfmt support an identation spec. This spec could also support vertical alignment, I suppose.

lread 2021-05-01T20:28:32.248100Z

Criterium is careful to warm up the JVM before benchmarking, whereas with GraalVM native-image… well…

lread 2021-05-01T20:32:59.248400Z

I suppose I could repeat the tests under a GraalVM native-image and see what that tells us. I’d rip out criterium for that, as I expect it does not make sense for GraalVM.

ericdallo 2021-05-01T20:34:32.248600Z

I don't think it worths, I definitely saw that performance issue without using a native binary since most of the time I'm using a jar during clojure-lsp development 😅

lread 2021-05-01T20:34:54.248800Z

Ah good to know.

lread 2021-05-01T20:35:03.249Z

Do the JVM times seem reasonable though? Forgetting hardware differences for a moment, if reformat took ~150ms for your provided scenario, would that be acceptable?

ericdallo 2021-05-01T20:36:12.249200Z

Yes, totally, the issue when it happens, takes a lot of time, I already saw taking 25s on the clj-jondo project 😔

lread 2021-05-01T20:37:01.249400Z

Woah, it would be cool if we had a reproducible case of that!

lread 2021-05-01T20:38:48.249600Z

So… a culprit in sporadic slowness can be garbage collection pauses…. have you looked into that at all?

lread 2021-05-01T20:39:19.249800Z

Or have you noticed any patterns at all?

ericdallo 2021-05-01T20:40:29.250Z

No, I didn't, sorry, I'll make some tests with clj-jondo project to check If I can find some kind of repro :)

lread 2021-05-01T20:41:01.250200Z

Cool, if a repro would be awesome, then we can dig in!

borkdude 2021-05-01T20:48:10.250400Z

What is clj-jondo?

2😂