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

Allow fipp to operate independently of core print options #56

Merged
merged 7 commits into from
Oct 3, 2019

Conversation

cichli
Copy link
Contributor

@cichli cichli commented Jan 21, 2019

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:

  • In visit-string and visit-character, bind *print-readably* to true when calling pr-str.
  • In visit-number, just use str, since that's what print-method does (print-dup adds the ctor).
  • In visit-pattern, just use str, since tag is always a symbol.

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
Copy link
Owner

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

@brandonbloom brandonbloom merged commit d40fdb5 into brandonbloom:master Oct 3, 2019
@brandonbloom
Copy link
Owner

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.

@brandonbloom brandonbloom mentioned this pull request Apr 20, 2020
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