brandonbloom / fipp

Fast Idiomatic Pretty Printer for Clojure
525 stars 44 forks source link

pprint does not print default tagged literals/elements #44

Closed mikub closed 7 years ago

mikub commented 7 years ago

Hi, I am having troubles making the fipp pretty print behave the same way as the regular cljs or clj print function when it goes down to tagged elements that are by default used to denote custom records.

See example below - pr, pr-str, print and print-str functions all use the default approach toward creating (edn-like) tagged element for MyRecord and all produce the same result:

cljs.user.MyRecord{:value "a"}

On the other hand fipp pprint (both clojure as well as edn/pprint) just errors out (I have tested in cljs only so far): cljs.user=> (defrecord MyRecord [value]) cljs.user/MyRecord cljs.user=> (pr (MyRecord. "a")) #cljs.user.MyRecord{:value "a"} nil cljs.user=> (fipp.clojure/pprint (MyRecord. "a")) #object[Error Error: Assert failed: (symbol? tag)]

Thanks & Regards Miro

brandonbloom commented 7 years ago

Hm, your example works for me.

Please let me know:

1) the stack trace 2) your Clojure Version 3) And if fipp.edn/pprint works (which is preferable to fipp.clojure/pprint when just printing data)

mikub commented 7 years ago

Thanks Brandon, will retest and send you those. Btw I tested only in clojurescript - i will test also in clojure to see whether the behavior is consistent.

Cheers Miro

brandonbloom commented 7 years ago

Haven't heard from you, so going to close this. Feel free to re-open if you have a repro. Thanks!

mikub commented 7 years ago

Hi Brandon, got to a point again where i need this functionality before https://dev.clojure.org/jira/browse/CLJS-1753 is resolved. I am still having this issue and looking at the code it seems to me that this really cannot work in CLJS. Testing with 0.6.7 but look at the repo it seems this part haven't changed in 0.6.9. So assuming the behaviour would be the same.

the stack trace is:

cljs.user=> (fipp.clojure/pprint (MyRecord. "a"))
#object[Error Error: Assert failed: (symbol? tag)]
   cljs.core/tagged-literal (jar:file:/home/miro/.m2/repository/org/clojure/clojurescript/1.7.28/clojurescript-1.7.28.jar!/cljs/core.cljs:32505:8)
   fipp.ednize/record->tagged (jar:file:/home/miro/.m2/repository/fipp/fipp/0.6.7/fipp-0.6.7.jar!/fipp/ednize.cljs:77:4)
   fipp.edn.EdnPrinter.prototype.fipp$visit$IVisitor$visit_record$arity$2 (jar:file:/home/miro/.m2/repository/fipp/fipp/0.6.7/fipp-0.6.7.jar!/fipp/edn.cljc:87:18)
   fipp.visit/visit-record (jar:file:/home/miro/.m2/repository/fipp/fipp/0.6.7/fipp-0.6.7.jar!/fipp/visit.cljc:29:18)

Looking at your code i see a bug in

(defn record->tagged [x]
  (tagged-literal (s/split (-> x type pr-str) #"/" 2)
(into {} x)))

If you look at the tagged-literal function, it requires symbol as a first argument.

(defn tagged-literal
  [tag form]
  {:pre [(symbol? tag)]}
  (TaggedLiteral. tag form))

In your code, (s/split (-> x type pr-str) #"/" 2) will never resolve to a symbol, but to a vector of two strings. Hence the assertion error:

cljs.user=> (clojure.string/split (-> (->MyRecord "a") type pr-str) #"/" 2)
["cljs.user" "MyRecord"]
cljs.user=> (symbol? (clojure.string/split (-> (->MyRecord "a") type pr-str) #"/" 2))
false

To fix this, you need to convert the vector to symbol:

cljs.user=> (apply symbol (clojure.string/split (-> (->MyRecord "a") type pr-str) #"/" 2))
cljs.user/MyRecord
cljs.user=> (symbol? (apply symbol (clojure.string/split (-> (->MyRecord "a") type pr-str) #"/" 2)))
true

So imho the fn should look like this:

(defn record->tagged [x]
  (tagged-literal  (apply symbol (s/split (-> x type pr-str) #"/" 2))
                   (into {} x)))

I can create a pull request for this, but havent tested it.

Cheers Miro

mikub commented 7 years ago

opps cannot create a pull request nor re-open this, probably since i am not a contributor. Will wait for awhile and then i may also re-submit this as a new issue.

brandonbloom commented 7 years ago

I've reopened because that does indeed look like a bug. I'll try your suggested change in a little bit.

brandonbloom commented 7 years ago

Fix deployed as version 0.6.10