Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print unresolved recursive values as their defining expressions, instead of ?? #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

onetom
Copy link

@onetom onetom commented May 14, 2021

Problem

While developing rmaps in a REPL session, they are printed with all the values appearing as ??, which makes debugging hard.

The following 3 expressions:

  @(def my-map
     {:foo 1
      :bar (rval (inc (ref :foo)))})

  @(def my-map
     (rmap {:foo 1
            :bar (inc (ref :foo))}))

  (valuate! my-map)

yield this output:

=> {:foo 1, :bar ??}
=> {:foo ??, :bar ??}
=> {:foo 1, :bar 2}

Solution

Store the rval-computing expressions (as metadata on their underlying function) and print those expressions, instead of ??.

The same 3 expressions as above, will be printed as:

=> {:foo 1, :bar (rval (inc (ref :foo)))}
=> {:foo (rval 1), :bar (rval (inc (ref :foo)))}
=> {:foo 1, :bar 2}

While the printed values happen to be homoiconic, it was not a design goal.

…s `??`

so it's easier to develop recursive maps.
@onetom onetom changed the title Print unresolved recursive values as their defining expression Print unresolved recursive values as their defining expressions, instead of ?? May 14, 2021
@onetom
Copy link
Author

onetom commented May 14, 2021

@aroemers I would update the README too and add some tests, but first, I would like to know if you see any problem with the feature itself or with the implementation approach.

I have the gut feeling that there was some strong reason for just using ??, but I couldn't figure out what, though I've read all your articles on the topic, multiple times.

@aroemers
Copy link
Owner

aroemers commented May 14, 2021

Hi @onetom,

Thank you for your PR and clear description. Know that I am pondering about your idea. Something that comes to mind is maybe printing it as #rmap/rval (inc (ref :foo)), and add an accompanying reader literal for it. This would make it more clear that it is not simply a quoted list.

@onetom
Copy link
Author

onetom commented May 14, 2021

We were thinking about using rmap in ClojureScript too and not sure what complications would reader tags impose.

Apart from that, it makes unevaluated values stand out better, so that's undoubtedly helpful.

I was also wondering, if it would be possible to have a simpler implementation of rmap, which doesn't utilize custom types, so it could run on Babashka out of the box too. 🤔

@aroemers
Copy link
Owner

We were thinking about using rmap in ClojureScript too and not sure what complications would reader tags impose.

Me neither, I don't use ClojureScript all that much. I don't have plans to support it myself, and I don't think it'll work out of the box as it is now. That said, printing something as a reader tag should not hinder ClojureScript I guess?

Apart from that, it makes unevaluated values stand out better, so that's undoubtedly helpful.

It took more work than expected, but I managed to create the #rmap/rval reader tag. You can have a look at the result here: https://github.com/aroemers/rmap/tree/feature/rval-tags. Let me know what you think.

I was also wondering, if it would be possible to have a simpler implementation of rmap, which doesn't utilize custom types, so it could run on Babashka out of the box too. 🤔

It should certainly be possible, though I'm not sure whether the implementation would become simpler. On the other hand, printing recursive values nicely would become more difficult. Babashka does support defrecord, but overriding its printing is not as easy as adding Object#toString to the record or provide a dispatch value for the print-method multimethod.

I'm open for ideas :)

@@ -64,7 +64,8 @@
not evaluated yet. The body can use the [[ref]] function while it is
evaluated, or you can bind it locally for use at a later stage."
[& body]
`(RVal. (fn [ref#]
`(RVal. ^{:ref/sexp '~@body}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unquote-splice would make the macro fail if the body contains multiple expressions like (rval (println "lazy!") "hi").

@onetom
Copy link
Author

onetom commented May 23, 2021

Thank you for the rval-tags experiment. I was trying to implement it myself last Thursday and Friday, but I was tripped over by not excluding clojure.core/ref in the namespaces, where I did an [rmap.core :refer :all]. I see you have added that exclusion to the rmap.core-test NS too. I think the documentation should give an example of this, because the error message I got as a consequence, was not very obvious.

Alternatively, ref could be renamed to something, which doesn't collide with clojure.core at least. For example, juxt/clip uses clip/ref or juxt.clip.core/ref to denote references, when it reads expressions as data, but they had to hardwire these specific symbols here: https://github.com/juxt/clip/blob/master/src/juxt/clip/impl/core.cljc#L64-L67

The best I could come up with is $, which is a symbol quite often used for denoting references. Some assembly dialects use it, Bash, Perl, PHP, jQuery, https://github.com/mfornos/clojure-soup...

Then the reader tags could be rmap/rval and rmap/$, resulting in

{:a 1 :b #rmap/rval (inc #rmap/$ :a)}

or when using [rmap.core :refer :all], just

{:a 1 :b (rval (inc ($ :a))}

but using [rmap.core :as rmap] would also look alright; almost like the reader-tag variant, just with more explicit boundaries:

{:a 1 :b (rmap/rval (inc (rmap/$ :a)))}

I will experiment with this rval-tags branch during the week and report back next week, the latest.

@onetom
Copy link
Author

onetom commented Oct 7, 2021

I just got back to this feature finally.
Successfully converted our configuration map into an rmap and it was observable along the way as expected.

I see there are still certain cases, when ?? are printed:

@(def my-data-map {:foo 1 :bar #rmap/ref :foo})
;=> {:foo 1, :bar #rmap/ref :foo}

@(def my-rmap (rmap my-data-map))
;=> {:foo #rmap/rval ??, :bar #rmap/rval ??}

but I saw in your previous commit how this is actually an improvement over the previous

{:foo #rmap/rvalv__1727__auto__, :bar #rmap/rvalv__1727__auto__}

I think the rval-tags branch is worthy of being merged to master.

@onetom
Copy link
Author

onetom commented Oct 7, 2021

Having the following definition for rmap* would get rid off those ??s:

(defn ^:no-doc rmap* [m]
  (reduce-kv (fn [a k ??] (assoc a k (RVal. (.f (rval ??)) ??))) m m))

However, I found that this function is not covered by the test suite.
All tests pass, even if I define rmap* as (def rmap* identity).
I'm a bit unclear on the use-case for this branch of the (if (or (map? m) (vector? m)) ... condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants