brandonbloom / fipp

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

Allow fipp to operate independently of core print options #56

Closed cichli closed 4 years ago

cichli commented 5 years ago

In some tooling contexts it is undesirable to read or rebind dynamic vars like *out*. For example, during the print phase of an evaluation at the REPL, using *out* can result in debug output being intermingled with the printed output.

This is also undesirable:

user> (binding [*print-dup* true]
        (fipp.edn/pprint [1 2 3]))
"""[""1"" ""2"" ""3""]"
nil

This PR adds a :writer option (defaults to *out*) and alters pprint-document to write its output directly to the writer rather than using the machinery of print. println is still called at the end to preserve the existing behaviour wrt *flush-on-newline*. N.B. this change seems to have a positive effect on the :long benchmark – from 126ms to 83ms on my machine.

For CLJS the equivalent option is :print-fn (defaults to *print-fn*), which I have added in a separate commit. *out* isn't generally set to anything in CLJS so I think this is the more logical extension point.

I also found that because of the use of pr-str in fipp.edn, it was possible to print invalid EDN depending on the values of *print-dup* / *print-readably*:

user> (binding [*print-dup* true]
        (fipp.edn/pprint (Integer. 1)))
#=(java.lang.Integer. "1")
nil
user> (binding [*print-readably* false]
        (fipp.edn/pprint ["a" "b" "c" \d \e]))
[a b c d e]
nil

Changes in fipp.edn to ensure we always print valid EDN:

Additionally, add the [javax.xml.bind/jaxb-api "2.3.1"] dev-time dependency, so the CLJS tests can run on JDK11. We could also bump the CLJS version to 1.10.63 or later.

brandonbloom commented 5 years ago

Thanks. Just letting you know that I saw this, but won't get around to reviewing it for a little while.

brandonbloom commented 4 years ago

Thanks for your patience on this. I've merged your changes. Not super excited about the complexity on this, but it all seems inherited from upstream & the feature makes sense to me. Thanks for the contribution.