Clojurians Log v2

Welcome @holyjak 👋 !

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

It’s a pretty cool library for sure!

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

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

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

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)

@lee PR welcome to try it on carve

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

PR to rewrite-edn also welcome

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

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

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

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

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

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

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

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

Yep, cljfmt works.

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

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

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

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.

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

Ah ok, your initial guess was right!

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

I switched to it.

from?

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

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

if that’s what you are getting at.

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

Oh gotcha…

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

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

Thanks @borkdude, looking foward to more feedback.

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.

This is why it surprised me

thanks

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

Reading for the second time: Typo: > redundent

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

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

Maybe you could show it using an example

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?

Good feedback, thanks.

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.

Eg. tag string …

Already did sexpr

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

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

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

analysis is now done by clj-kondo

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

Might be interesting

Nope just a current release.

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

Or maybe I did with rewrite-cljc ages ago…

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

so basically: to support more than one bit of info

which a map is a good choice for

Does the number of children change for namespaced maps?

Nope

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

assuming that it returns the prefix?

Yeah, there are examples in the change log.

or does it return only the k/vs?

Oh right from a node API perspective…

You’ll get the prefix

Like in v0

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

without zippers

I guess I can just try this myself in the REPL

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.

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

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

cool, it does something :)

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

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

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

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

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.

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

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

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

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

But it might help more in CLJS, who knows

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

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

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

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

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

yep, very nice

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

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

Sure, yep.

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

What do you mean with the word precious here?

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

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

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

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

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

I did consider it.

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

It’s all quite subjective though…

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

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

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

Yes very true.

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!

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

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

So what was the bug?

An open source journey begins with but one bug

And is it fixed?

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.

so cljfmt is using cljs?

Yeah it supports both clj and cljs.

ah, .cljc ok

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

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

Yes!