Clojurians Log v2

Hey @borkdude, I got a chance to slim down namespaced elements design notes for rewrite-cljc. Some details are sure to change as I move to implementation, but would love to have a critical eye, when you have a moment, to review for problems/opportunities. Content is on same branch but moved to a https://github.com/lread/rewrite-cljc-playground/blob/lread-ns-kw-map/doc/design/namespaced-elements.adoc for, hopefully, a less overwhelming review experience.

Am happy to get feedback from any others with an interest too! ❤️

@lee with: > Same as 3 but also ensure backward compatibility with current rewrite-clj implementation you should keep an eye on binary size with GraalVM binaries as this relies on *ns*. I'm not a big fan of using that at runtime in GraalVM binaries as this can result in bloat.

> but automatically parsing and returning a technically correct sexpr for auto-resolved namespaced elements is a rabbit hole that we’ll reject for now. agreed, I'll skip over the rabbit hole part

yeah rabbit hole section is summary of you and I discussing this ages ago.

> fall back to ns if the current namespace is not specified by caller. same comment as above, should be careful about this

Noted, thanks. Maybe I’ll do some sort of http://grep.app to try to learn if any public repos that use rewrite-clj are even using this.

Or… it could just be a documented breaking change.

About the *ctx*: have you also considered just passing an extra opts map to sexpr? Dynamic vars do not work well with laziness. Passing in explicit opts is often preferred:

(sexpr foo opts)

I once did a massive refactor on a commercial project rewriting dynamic vars to explicit opts because of laziness problems

Hmm… interesting. I did consider it but sexpr is also used internally. I could have a look at impact.

you can make an extra arity of course, opts being optional

ya, makes sense. I knew that passing over dynamic vars was preferred but did not know about laziness problems. I’ll take a gander.

About creating map nodes: the interface looks fine to me, but I'd also like to see the same record type backing both types of maps, so writing predicates for map? nodes becomes trivial

This is my feedback.

Yep, plan is to have MapNode now also cover namespaced maps.

A breaking change seems fine with respect to sexpr and namespaced maps. I think this was one of the more recent features in rewrite-clj and it feels more like it got bolted on (with all respect)

Yeah I’m cool with totally breaking namespaced maps, was more concerned about breaking namespaced keywords.

Do you have any comments on https://github.com/lread/rewrite-cljc-playground/blob/lread-ns-kw-map/doc/design/namespaced-elements.adoc#sexpr-behaviour? I’m wishy washy on last 2 behaviours.

One problem I ran into with clj-kondo and *ns* was, when sexpr-ing namespaced maps from completely unrelated code (clj-kondo lints any code) you have to create namespaces to be able to get those. Which is crazy.

Yeah, understood.

the behavior of sexpr based on your config map seems ok to me. I do a similar thing here in edamame:

(parse-string "[::foo ::str/foo]" {:auto-resolve '{:current user str clojure.string}})
;;=> [:user/foo :clojure.string/foo]

cool, what does edamame do when alias str is not provided?

I think some breaking changes are justified and if you are going to make them, it will be best to make the in the first release of your new lib (and document them somewhere).

> cool, what does edamame do when alias str is not provided? exception

but it also supports a default I think

let me check

user=> (e/parse-string "::foo" {:auto-resolve (constantly 'foo)})
:foo/foo

user=> (e/parse-string "::s/foo" {:auto-resolve (constantly 'foo)})
:foo/foo

user=> (e/parse-string "::s/foo" {:auto-resolve (constantly nil)})
Execution error (ExceptionInfo) at edamame.impl.parser/throw-reader (parser.cljc:92).
Alias `s` not found in `:auto-resolve

Interesting.

So I’d likely setup something similar for rewrite-cljc but default it to something that never throws.

It was designed with a map of options in mind first, but later on this was made more flexible, so you can just pass in a function of one arg

I found my initial stab underwhelming here - at alias not found behaviour:

(binding [*ctx* {:ns-aliases {'a1 'another.ns.a1
                             'str 'clojure.string}]
  (sexpr (parser/parse-string "::nope/foo"))
  ;; => :_<?namespace-not-found?>_/foo
  (sexpr (parser/parse-string "#::nope{:a 1 :b 2}"))
  ;; => {:_<?namespace-not-found?>_/a 1 :_<?namespace?>_/b 2}
)

@lee You could do this:

user=> (e/parse-string "::s/foo" {:auto-resolve (fn [alias] (get {:current 'foo 'str 'clojure.string} alias 'unresolved))})
:unresolved/foo

yeah that could work too:

user=> (e/parse-string "::s/foo" {:auto-resolve (fn [alias] (or (get {:current 'foo 'str 'clojure.string} alias) (symbol (str alias "-unresolved"))))})
:s-unresolved/foo

not a bad idea

ah… interesting.

and probably not a terrible default (I think the majority of folks will not provide any namespace options).

Should I be more obscure with my default unresolved? Instead of :s-unresolved/foo would :<?s-unresolved?>/foo be silly?

Anyway, thanks @borkdude, in summary to recap your major points: 1. consider not supporting *ns* for backward compatibility. Not only is is a bloater for GraalVM, it is also an awkward mechanism for, at least this, use case. 2. consider passing an optional opts over using dynamic vars for namespace info. Dynamic vars can cause issues with lazyness. 3. in general, consider breaking compatibility where things are obviously amiss.

well summarized!

Thanks! I’ll take another stab tomorrow and maybe start implementing to feel out issues.

And, as always @borkdude, thanks for your beautiful big brain and kind heart!

Likewise!