brandonbloom / fipp

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

Visit Eductions as sequences #46

Closed mfikes closed 6 years ago

mfikes commented 7 years ago

Without a change like the one in this PR, Eductions (non-pretty) print as sequences, but in Fipp, it goes down a path that prints the object.

user=> (def x (eduction (map identity) (range 20)))
#'user/x
user=> x
(0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19)
user=> (require '[fipp.edn :refer (pprint) :rename {pprint fipp}])
nil
user=> (fipp x {:width 10})
#object[clojure.core.Eduction
        "0x77cf277b"
        "clojure.core.Eduction@77cf277b"]
nil

The change simply detects this situation during visiting and converts via sequence, thus producing:

user=> (fipp x {:width 10})
(0
 1
 2
 3
 4
 5
 6
 7
 8
 9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19)
nil

A similar, thing occurs with ClojureScript.

It is not clear at all to me if this is the right place or approach to address something like this, or whether it would be more appropriate for it to go into something like the cases handled in fipp.ednize, for example, so feel free to take this PR as inspiration (or reject it of course, if there is a reason not do pretty-print Eduction instances as sequences).

FWIW, I'm soaking a similar change downstream in Planck to see if anything breaks when trying this: https://github.com/mfikes/planck/commit/363db3edd2b23305c513154242b4c72d9f5a22cb

brandonbloom commented 7 years ago

Hm, odd, seems like println prints an object but prn and clojure.pprint/pprint prints as a sequence.

I have two (small) concerns:

  1. Maybe you wouldn't want to force realization of an eduction. They could be potentially be effectful to avoid side effects with laziness.
  2. The only other things that print as a bare sequences satisfy the seq? predicate. If you ignore the first concern, I would have expected something like #eduction (...) instead of just (...).

Barring these concerns, I don't have a strong feeling either way. If you tell me that is useful and has caused no issues, I'm inclined to believe you. What prompted this patch?

whether it would be more appropriate for it to go into something like the cases handled in fipp.ednize

Yeah, the -edn method is probably more appropriate.

mfikes commented 7 years ago

@brandonbloom What prompted the patch is that, in Planck with Fipp pretty-printing, something like (range 50) will be pretty printed vertically (with each number being syntax-highlighted via the hooks Planck has set up). But, (eduction (map identity) (range 50)) doesn't pretty print. It shows up as a horizontal sequence, w/o syntax-highlighting, etc.

So, I patched this up in Planck master, with an branch in visit-unknown for this case, and then started thinking: "Wait, maybe Fipp could or should handle this."

The approach Planck is taking is fine with me—perhaps changing Fipp in this way would break other Fipp clients that are accustomed to the existing behavior.