Fork me on GitHub
#rewrite-clj
<
2021-03-09
>
lread19:03:10

Welcome @holyjak 👋 !

❤️ 1
holyjak19:03:03

Thank you! After @borkdude's praise I could not not check out #rewrite-clj :)

lread19:03:20

It’s a pretty cool library for sure!

lread19:03:44

I think I am pretty much ready to merge the clj-commons/rewrite-clj v1 branch to main. After that, I’ll work toward getting a release up on clojars. Before I do the merge anybody want to review the https://github.com/clj-commons/rewrite-clj/blob/v1/CHANGELOG.adoc#rewrite-clj-v1? I’m looking for anything that makes you think, “uh oh, that will be a problem”.

❤️ 2
vemv19:03:56

I could try these changes, maybe on the weekend? An alpha would help. I have a rewrite-clj program that used/hacked things for getting namespaced keyword support. It could be a useful report, verifying if I was able to update rewrite-clj->rewrite-cljc with only moderate pain (I do expect at least some little surprise due to my hacking)

lread20:03:07

Cool, yeah, thanks, I think the real feedback can only come when people actually try out the first alpha.

holyjak19:03:09

Do I understand correctly you are rewriting #rewrite-clj? 😹

lread19:03:05

Oh ha! That’s very meta! I do use rewrite-clj to generate rewrite-clj’s public APIs. Which is fun.

🔥 1
borkdude19:03:16

@lee PR welcome to try it on carve

borkdude19:03:42

clj-kondo has a fork of rewrite-clj so that won't really work for testing

borkdude19:03:31

PR to rewrite-edn also welcome

borkdude19:03:25

I know I want to upgrade those libs to the clj-commons one anyway, so might as well get it over with now. Been using your playground so far

lread19:03:36

Yeah, rewrite-clj v1 branch, is pretty much equivalent to my rewrite-cljc-playground with more fixes and some minor changes to namespaced elements.

lread20:03:19

Once I get the alpha out on clojars, I’ll happily do PRs for carve and rewrite-edn.

borkdude20:03:00

cool, then I can also have a clojars release of rewrite-edn. which I already did but I forgot I have a git dep :P

lread20:03:36

BTW, I am already running carve and rewrite-edn tests against rewrite-clj v1 via its libs tests.

borkdude20:03:09

Awesome. I also saw (not read entirely) the conversations with zprint. I assume you also test clj-format?

lread20:03:13

Yeah, I’m not exactly sure how antq manages a clojars release when it is using a git dep to rewrite-cljc-playground.

lread20:03:26

Yep, cljfmt works.

borkdude20:03:39

Maybe he just copies your sources into the target dir before jar building? :P

borkdude20:03:50

Maybe @slipset knows what happens when you deps-deploy a library with a gitlib to clojars?

lread20:03:31

Yeah, I dunno, I figured the dep would have to be in pom?

lread20:03:10

I’m not sure how long it has been since you had a peek at v1 namespaced elements changes @borkdude, it’s similar to what we discussed ages ago, but if you have a time for a https://github.com/clj-commons/rewrite-clj/blob/v1/CHANGELOG.adoc#rewrite-clj-v1 that’d be great.

borkdude20:03:15

@lee It seems he does package your code along with his jar ...

lread20:03:37

Ah ok, your initial guess was right!

borkdude20:03:50

@lee what is: > Potentially breaking: clojure.tools.reader.edn

lread20:03:01

I switched to it.

lread20:03:41

in some places I think clojure.edn? Would have to look it up.

lread20:03:11

Oh I see… maybe more of a bug fix than something to be listed under breaking changes…

lread20:03:21

if that’s what you are getting at.

borkdude20:03:14

I mean: you switched to Y. Yes, ok, but from what X? Where did you switch from? It raises more questions than answers to me ;)

lread20:03:29

Oh gotcha…

borkdude20:03:47

I'll have a more serious read later, kind of in the middle of something

lread20:03:06

Thanks, I’ll get more elaborate. Might be a false old note, will double check.

lread20:03:56

Thanks @borkdude, looking foward to more feedback.

lread20:03:26

I thought rewrite-clj v0 used read-string from other namespaces - other than from tests, this is not the case, I’ll delete the note.

borkdude20:03:46

This is why it surprised me

borkdude20:03:14

Luckily rewrite-clj didn't because I would not have been able to use it with GraalVM at the time and clj-kondo wouldn't have happened and then babashka probably neither

borkdude20:03:41

Reading for the second time: Typo: > redundent

👍 1
lread20:03:21

Ya, I think I actually temporarily introduced a bad read-string due to a booboo in transcribing prefixed requires.

borkdude20:03:51

The thing that I wonder most about after reading it is this one: > Namespace map prefix, is now stored in a namespaced map qualifier node. Prior to v1, was stored as a keyword. You give a lot of examples of any combination of things, but you call sexpr on everything, while I was wondering what the difference for this map prefix was

borkdude20:03:42

Maybe you could show it using an example

borkdude20:03:08

And the other thing: does this affect how you call :children on a namespaced map node, what you get back and how you should process it?

lread20:03:42

Good feedback, thanks.

lread20:03:01

Thinking out loud here: I’m still a bit confused as to what is and is not the public API. For example node record fields were not documented in v0. This indicates they are an implementation detail. This will change if we support serialization of nodes, but for now they are considered internal. So I think I’ll express differences between the old an new namespaced map prefix from the perspective of the public node API.

lread20:03:57

Eg. tag string

lread20:03:04

Already did sexpr

borkdude20:03:17

In clj-kondo I do rely on this, but I'm using my own fork anyway. so is clojure-lsp I think

lread20:03:01

I actually run clojure-lsp’s tests against rewrite-clj v1, they pass.

borkdude20:03:23

That's good. Maybe this is because they are not doing analysis with rewrite-clj anymore, but only rewriting

borkdude20:03:30

analysis is now done by clj-kondo

borkdude20:03:46

Have you tested older versions that didn't use clj-kondo yet?

borkdude20:03:56

Might be interesting

lread20:03:20

Nope just a current release.

borkdude20:03:36

What is the reason you changed this field from a keyword to a map?

lread20:03:47

Or maybe I did with rewrite-cljc ages ago…

lread20:03:19

For clarity and to support current-ns auto-resolve :: , ex. #::{:a 1} which rewrite-clj v0 did not support at all.

borkdude20:03:44

so basically: to support more than one bit of info

borkdude20:03:54

which a map is a good choice for

borkdude20:03:29

Does the number of children change for namespaced maps?

borkdude20:03:58

And what happens when you call (sexpr (first (:children namespaced-map))) ?

borkdude20:03:06

assuming that it returns the prefix?

lread20:03:39

Yeah, there are examples in the change log.

borkdude20:03:42

or does it return only the k/vs?

lread20:03:05

Oh right from a node API perspective…

lread20:03:24

You’ll get the prefix

lread20:03:29

Like in v0

borkdude20:03:40

Yes, I manipulate a lot of the nodes just by hand, by iterating over the :children, etc

borkdude20:03:44

without zippers

borkdude20:03:24

I guess I can just try this myself in the REPL

lread20:03:29

Yeah, it is a bit awkward when you want to treat a map as a map whether it be a namespaced-map-node or a map-node.

lread20:03:26

But v1 node traversal matches v0, and I like that. I have an https://github.com/clj-commons/rewrite-clj/issues/131.

borkdude20:03:50

user=> (rewrite-clj.node/sexpr (first (:children (p/parse-string "#::{:a 1}"))))
?_current-ns_?

borkdude20:03:08

cool, it does something :)

lread20:03:12

yep, uses the default auto-resolve function which the user can choose to override.

borkdude20:03:51

user=> (rewrite-clj.node/sexpr (first (:children (p/parse-string "#::{:a 1}"))) {:auto-resolve {':current 'foo}})
foo
yes, excellent

lread20:03:05

an idea I got from some clever fellow, can’t remember his name. simple_smile

😆 1
borkdude20:03:55

Last question: can you tell more about cljs perf things you removed to improve compatibility between both?

borkdude21:03:33

And lastly: lots of respect for your thorough work. This is an important piece of software for the clj ecosystem and you stepped up, a lot of thanks.

lread21:03:20

Yeah… I should actually provide a link to cljs perf because I have one somewhere… rundis made some efforts to optimize rewrite-cljs and wrote up a blog post. I dropped most of them in favour of a unified single code base for rewrite-cljs and rewrite-clj. Ah here it is: http://rundis.github.io/blog/2015/clojurescript_performance_tuning.html

lread21:03:14

Well, you and others have certainly been supportive, thanks for the continued feedback over the journey.

lread21:03:52

So rewrite-cljs users might notice a slowdown. I’ve not done any perf testing at all.

borkdude21:03:08

@lee I have done this in clj-kondo: > First quickwin - Moving away from multimethods Guess that perf improvement I saw in general usage? I can't say that I noticed.

borkdude21:03:19

But it might help more in CLJS, who knows

lread21:03:46

And this was against ClojureScript circa 2015, so… dunno what perf changes have happened since then.

borkdude21:03:52

I did find it a little bit weird to use multi-methods for a private namespace.

borkdude21:03:15

They do incur some perf overhead so switching to a closed set of cases does make sense

borkdude21:03:45

This is what I did in edamame, the parser which is largely inspired by the rewrite-clj parser

lread21:03:57

The nice thing is that is that we can improve performance if there are issues without breaking things.

borkdude21:03:11

yep, very nice

lread21:03:20

I put that under potentially breaking, because it might make rewrite-clj v1 cljs unusable if perf is really poor.

borkdude21:03:02

I guess if that is the case, the JVM can also benefit from some optimizations at the same time

lread21:03:17

Sure, yep.

lread21:03:24

I was also thinking about a prior conversation we had about version schemes and using commit count. Perhaps subjective, but I’m thinking a benefit of using commit count really makes the version seems less precious and makes cutting a release less precious (which to me is good).

borkdude21:03:33

What do you mean with the word precious here?

lread21:03:04

Less of an important event. If need to release for any reason, I release.

borkdude21:03:42

What does that have to do with the version number? Both can be scripted (I certainly use a script for this)

borkdude21:03:57

I mean, bumping the version is a script in my projects

lread21:03:25

I was thinking of more along the lines of psychological than technical.

borkdude21:03:32

If you really want to make your versions meaningless, use dates. clj-kondo uses http://yyyy.mm.dd

lread21:03:46

I did consider it.

borkdude21:03:20

I can explain why I did this for clj-kondo: this project will never be finished, so always just use the newest

lread21:03:37

It’s all quite subjective though…

borkdude21:03:55

For babashka I do want to indicate something along the lines of API breaking or important new functionality, so I use 0.2.13 now. When spec goes in it will certainly be 0.3.0 or so

lread21:03:33

Yeah I think that’s what rewrite-clj v0 was doing.

borkdude21:03:36

It is subjective yes. So it really doesn't matter much

lread21:03:59

Yes very true.

lread21:03:12

Ok, I’m going to go and incorporate your very helpful feedback into the change log and tomorrow will merge to main! Thanks as always!

lread21:03:28

Funny thing about rewrite-cljc and rewrite-clj v1… all I really wanted to do was fix a little bug in cljfmt… huh!

😂 1
borkdude21:03:10

Improving the world one bugfix... eh library at a time

borkdude21:03:50

So what was the bug?

lread21:03:53

An open source journey begins with but one bug

borkdude21:03:56

And is it fixed?

lread21:03:22

No, not yet. There were a few actually, I think https://github.com/weavejester/cljfmt/pull/77#issuecomment-470721499, but it was missing namespaced map support in rewrite-clj v0 and lotsa missing stuff in rewrite-cljs that really lit the flame.

borkdude21:03:14

so cljfmt is using cljs?

lread21:03:28

Yeah it supports both clj and cljs.

borkdude21:03:29

ah, .cljc ok

lread21:03:51

It references both rewrite-cljs and rewrite-clj v0.

borkdude21:03:47

So once migrating it to v1 you may finally get to it? ;)