Clojurians Log v2

Hi, @borkdude. Just a single question: I'm using SCI on ClojureScript, and want to pass ClojureScript records to, and from SCI. So, I saw the :readers option. But if I send an options like {'example.Error ex/map->Error}, when I evaluate the code #example.Error{:type "SomeError, :message "A Message"} it returns a PersistentArrayMap, not the record

@mauricio.szabo Do you want to read records from source code strings?

I can reproduce the problem:

cljs.user=> (type (sci/eval-string "#example.Error{:type \"SomeError\", :message \"A Message\"}" {:readers {'example.Error map->Error}}))
cljs.core/PersistentArrayMap

Made an issue here: https://github.com/borkdude/sci/issues/386

@mauricio.szabo Should be fixed with https://github.com/borkdude/sci/commit/bff3baa4114393abb7d24effdffd3b9d68b93dda

@mauricio.szabo Do you need a new mvn/version or do you use it as a git dep?

A mvn/version is better, because I'm using on Shadow-CLJS 😄. Thanks a lot!

(BTW, that was FAST! 😄)

@mauricio.szabo ok: borkdude/sci {:mvn/version "0.1.1-alpha.7"}

Btw, since yesterday night the CLJS tests for sci run really sloooow on my machine, no idea why

on CI I don't really observe this difference

I'm also observing this with 0.1.0 which was released quite a while ago. I guess my holiday laptop (Macbook Air) just doesn't cut it anymore, but the JVM tests are still super fast.

@borkdude well, everything seems normal here on my machine with SCI tests... maybe some node/macOS update?

Hey, I'm trying to use clojure.walk/macroexpand-all inside of sci but it's failing with the classic Caused by: <http://java.io|java.io>.FileNotFoundException: Could not locate clojure/spec/alpha__init.class, clojure/spec/alpha.clj or clojure/spec/alpha.cljc on classpath. Basically I'm running this:

(sci.core/eval-string
   "(macroexpand-all '(-> 1 println))"
   {:bindings {'macroexpand-all clojure.walk/macroexpand-all}})
;;=> (println 1)

Don't do that :)

😢

Why not?

Sci already has macroexpand. The macroexpand-all should call that macroexpand, not clojure's own macroexpand.

So I need to re-implement macroexpand-all in sci?

Yes, or contribute it to sci

All right, for the short-term would evaluating the implementation inside of sci be enough?

I think so yes

I'd love to contribute though 🙂

You can contribute by making an issue first :)

All right, well I actually already tried that. But I didn't get the result I expected:

(sci.core/eval-string "
(defn walk [inner outer form]
  (cond
   (list? form) (outer (apply list (map inner form)))
   (map-entry? form)
   (outer [(inner (key form)) (inner (val form))])
   (seq? form) (outer (doall (map inner form)))
   (record? form)
     (outer (reduce (fn [r x] (conj r (inner x))) form form))
   (coll? form) (outer (into (empty form) (map inner form)))
   :else (outer form)))

(defn prewalk [f form]
  (walk (partial prewalk f) identity (f form)))

(defn macroexpand-all [form]
  (prewalk (fn [x] (if (seq? x) (macroexpand x) x)) form))

 (macroexpand-all '(-> 1 println))")
;;=> (-> 1 println) ;; <<<<<<<<<<< INCORRECT

ok, let me try

oh I see. yes, well, that has a different reason

The -> is built in kind of as a primitive in sci, it was one of the first macros I implemented, when user-defined macros weren't possible yet

so it's not really expanded like other macros

Oh! I see. ->> does work

very interesting

I have trouble finding where exactly the implementation of that "special" macro is

ah, there it is, in analyzer.cljc:

;; TODO: implement as normal macro in namespaces.cljc
                -> (expand-> ctx (rest expr))
                ;; TODO: implement as normal macro in namespaces.cljc
                as-> (expand-as-> ctx expr)
                

https://github.com/borkdude/sci/blob/f6389378181b24b828566ac5307eca25aadf119e/src/sci/impl/analyzer.cljc#L27 Does this mean it's also applicable for as-> ?

yes

it's historical, not necessarily the way it has to be now

Maybe I can cook something up then

Sure

I think we already have tests for -> so if they keep working, you don't have to add new ones I think

Ah ok, I'll remove those (and update my branch)

or you can separate them out from the bug chunk of core tests and move them to their own deftest, which is cleaner I think

I see, I'll move them then

You can also add a test for macroexpanding it

Done, simple tests but they show what's expected

I just merged a huge PR (having to do with the new stack trace support in babashka), so there's a conflict now. Can you solve it? Can wait till tomorrow, since I'm calling it a day

Btw, one more thing: the namespaces-test namespace is intended for tests related to ns etc

So you can still stick your -> tests in core_tests.cljc but in a separate deftest

Ah right

All right, I'll fix that tomorrow 🙂

Or well might as well do it now lol

Anyway, good night! Calling it a day as well 👋