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

New print middleware #118

Merged
merged 6 commits into from Jan 29, 2019

Conversation

Projects
None yet
4 participants
@cichli
Copy link
Member

cichli commented Jan 10, 2019

Summary

Replaces the existing pr-values middleware with a new middleware: nrepl.middleware.print/wrap-print. This middleware is intended to provide generic printing functionality to other middlewares. By default its behaviour is entirely backwards-compatible with pr-values. Rather than being coupled to the specific key :value, the new middleware allows other ops to provide :nrepl.middleware.print/keys in responses.

See the doc updates in the PR for more details, but the new middleware's options are summarised below:

  • :nrepl.middleware.print/var – (namespaced) name of var to use as printing function
  • :nrepl.middleware.print/options – map of options to pass to printing function
  • :nrepl.middleware.print/stream? – whether or not to stream the result of printing each value over one or more messages
  • :nrepl.middleware.print/buffer-size – size of the buffer to use when streaming results; represents an upper bound on response size in bytes
  • :nrepl.middleware.print/quota – hard limit of number of bytes to print before truncating

An example of non-streamed vs. streamed printing:

-> {:op :eval
    :code "(range 512)"}
<- {:ns "user" :value "(0 1 2 3 ... 510 511)"}
<- {:status ["done"]}
-> {:op :eval
    :code "(range 512)"
    :nrepl.middleware.print/stream? 1}
<- {:value "(0 1 2 3 ... 282 2"}
<- {:value "83 284 ... 510 511)"}
<- {:ns "user"}
<- {:status ["done"]}

Clients have the choice of rendering each part of the result as they arrive, or using some buffering strategy (perhaps to prevent the output from being intermingled with that of *out* / *err*).

An example of truncation with streaming enabled:

-> {:op :eval
    :code "(range 512)"
    :nrepl.middleware.print/stream? 1
    :nrepl.middleware.print/quota 8}
<- {:value "(0 1 2 3"}
<- {:status ["nrepl.middleware.print/truncated"]}
<- {:ns "user"}
<- {:status ["done"]}

and without streaming enabled:

-> {:op :eval
    :code "(range 512)"
    :nrepl.middleware.print/quota 8}
<- {:ns "user"
    :value "(0 1 2 3"
    :status "nrepl.middleware.print/truncated"
    :nrepl.middleware.print/truncated-keys ["value"]}
<- {:status ["done"]}

Motivation

We should allow tools built on top of nREPL to minimise network transparency and allow a tight feedback loop where possible. Some computations are slow, and some are truly infinite – but if they compute their result incrementally, and we can print this result incrementally, we should provide a way of doing that. We should also allow the limiting of printing so as to not flood clients with excess data.

Implementation

session-out is close to what we need but it's coupled with the eval middleware by its use of *msg*. Rather than check the dynamic binding *msg* for which transport to use, we can instead close over the transport each time we eval. This might impact other middlewares which were capturing *out* from sessions and using it for their own purposes, but the new replying-PrintWriter fn provides a more sane way of doing this without relying on any implementation details. See the commit message of the first commit for more details.

The signature of the printer function is now [value writer options], regardless of if streaming is enabled. It doesn't match 1:1 with the signature provided by every pretty printing library out there but it should be possible to wrap any of them in a function that matches this signature (perhaps a library that does this for the most popular printing libraries would be useful in itself).

There is a further nuance to printer that I have tried to document slightly better: it shouldn't really use *out* at all. What if we call printer on some lazy value that prints to *out* when it's realised? It all gets intermingled in the same output stream. Note that preventing this intermingling was in fact one of the motivations for the development of prepl: see this comment. I'm not going to argue that making the writer argument explicit will always help in this case, though – see clojure.pprint/pprint, which has an arity that takes an explicit writer, yet still binds *out* internally...

user> (let [x (->> (range 5)
                   (map (fn [x]
                          (println x))))
            sw1 (java.io.StringWriter.)
            sw2 (java.io.StringWriter.)]
        (binding [*out* sw2]
          (clojure.pprint/pprint x sw1)
          {:sw1 (str sw1) :sw2 (str sw2)}))
{:sw1 "0\n1\n2\n3\n4\n(nil nil nil nil nil)\n", :sw2 ""}
@cichli

This comment has been minimized.

Copy link
Member Author

cichli commented Jan 10, 2019

I will rebase and resolve the conflicts after we decide what to do with printer.

@cichli cichli force-pushed the cichli:streamed-printing branch 2 times, most recently from 4649c90 to cce9c9c Jan 10, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 10, 2019

We now need a different contract of printer for the streamed and non-streamed cases. I disagree with the existing contract of printer. Why is it expected to return a string? Printers don't return strings, they write strings to streams (and of course, there are flush/close semantics which are lost with strings). If I really want a string then I'll give it a StringWriter. See: print-dup and print-method, the fundamental printing primitives in clojure.core. The contract is [x writer]. All of the printing convenience functions – println, pr-str, etc. – bottom out at these, generally calling them on either out or a StringWriter.

Well, this was the contract chosen as it was in line with was present - pr-str or print-dup simply returned the result printed to a string. I'm not arguing that streaming this is a better approach, but I didn't think much about this. So, what would be the ideal contract in your opinion?

There is a further nuance to printer that I have tried to document slightly better: it shouldn't really use out at all. What if we call printer on some lazy value that prints to out when it's realised? It all gets intermingled in the same output stream. Note that preventing this intermingling was in fact one of the motivations for the development of prepl: see this comment. I'm not going to argue that making the writer argument explicit will always help in this case, though – see clojure.pprint/pprint, which has an arity that takes an explicit writer, yet still binds out internally...

Yeah, I agree with you regarding this.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 10, 2019

I have to say that the implementation looks great to me! (but I'm having a headache right now, so probably I should re-read it tomorrow morning) :-)

I really like that you made this opt-in, which is a great approach.

The current API and contract are brand new and considered experimental, so we can technically change them, although I'd be careful with this as it's not clear how many people have implemented this already. I was just about the cut a new CIDER release tomorrow.

@cichli cichli force-pushed the cichli:streamed-printing branch 8 times, most recently from 79998a1 to 3d13b4e Jan 15, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 16, 2019

@cichli I see you've updated the branch, but there are still some merge conflicts.

@cichli cichli force-pushed the cichli:streamed-printing branch 2 times, most recently from 62f65fb to 77151c0 Jan 18, 2019

@cichli cichli changed the title Streamed printing New print middleware Jan 19, 2019

@cichli cichli force-pushed the cichli:streamed-printing branch 2 times, most recently from 99a0da0 to 73dd549 Jan 19, 2019

@cichli

This comment has been minimized.

Copy link
Member Author

cichli commented Jan 19, 2019

@cichli I see you've updated the branch, but there are still some merge conflicts.

All conflicts are now resolved – please see the updated PR description.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 19, 2019

Codecov Report

Merging #118 into master will increase coverage by 1.45%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   75.39%   76.85%   +1.45%     
==========================================
  Files          15       15              
  Lines        1207     1283      +76     
  Branches       58       51       -7     
==========================================
+ Hits          910      986      +76     
- Misses        239      246       +7     
+ Partials       58       51       -7
Impacted Files Coverage Δ
src/clojure/nrepl/middleware.clj 85.03% <ø> (ø) ⬆️
src/clojure/nrepl/transport.clj 59.25% <100%> (+3.25%) ⬆️
...rc/clojure/nrepl/middleware/interruptible_eval.clj 97.14% <100%> (+2.29%) ⬆️
src/clojure/nrepl/middleware/session.clj 90.69% <100%> (+1.86%) ⬆️
src/clojure/nrepl/middleware/print.clj 93.44% <93.44%> (ø)
src/clojure/nrepl/misc.clj 80.76% <0%> (-15.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ee6b77...2ddf536. Read the comment docs.

@cichli cichli force-pushed the cichli:streamed-printing branch 7 times, most recently from 37118ab to fbb9b4f Jan 19, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 23, 2019

Hmm, that’s weird. I’ve sent you an invitation to join before I started my vacation. Perhaps you didn’t see it? I just confirmed you’ve got an outstanding invitation to join the nREPL org.

Btw, I’d suggest cutting 0.6 relatively quickly after merging this due to it’s importance. If I were you’d I’d consider for inclusion in the release also #116, #107, #87 and #67 depending on their current state.
Afterwards we’ll follow up with another release focused on the big improvements @cgrand has been working on - e.g. #97, #96 and #106.

@cichli

This comment has been minimized.

Copy link
Member Author

cichli commented Jan 23, 2019

Whoops, seemed I missed the notification. All sorted now – thanks! I will take a look at what can be merged of those four PRs today.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 23, 2019

Fantastic! Just one small tip for the release - don’t forget to update the version in the antora descriptor before tagging it, otherwise Antora won’t generate the docs for 0.6 properly. (It’s master most of the time, but it has to be to a concrete version number at release time)

@cichli

This comment has been minimized.

Copy link
Member Author

cichli commented Jan 23, 2019

Sure thing – I'll take a look at the commits for the previous release and replicate it. Hope you're enjoying the vacation!

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 24, 2019

I sure am! 😃

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 25, 2019

One small thing I remembered about this - it will be good to bundle some print fn based on pprint (I think it doesn’t accept a map of options), so users would easily be able to configure some pretty-printing out of the box. Probably adding a couple of examples about using fipp and zprint in the docs would be helpful for end users as well.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 25, 2019

And on a related note - I think print-fn will be a better name than print which is a bit too generic.

@cichli cichli force-pushed the cichli:streamed-printing branch from 23ae70e to 26a2001 Jan 26, 2019

@cichli

This comment has been minimized.

Copy link
Member Author

cichli commented Jan 26, 2019

One small thing I remembered about this - it will be good to bundle some print fn based on pprint (I think it doesn’t accept a map of options), so users would easily be able to configure some pretty-printing out of the box.

I have added a new commit which I think will make configuring this a lot easier for many users. The print middleware namespace now contains the following dynamic vars which can be set! at the REPL:

  • *print-fn*
  • *stream?*
  • *buffer-size*
  • *quota*

So it's possible to do:

user=> (set! nrepl.middleware.print/*quota* 32)
32
user=> (range)
(0 1 2 3 4 5 6 7 8 9 10 11 12 13
user=>
user=> (set! nrepl.middleware.print/*print-fn* clojure.pprint/pprint)
#object[clojure.pprint$pprint 0x2bc11aa0 "clojure.pprint$pprint@2bc11aa0"]

user=> (meta #'int)
{:added "1.0",
 :ns #object[clojure.lang.Namespace 0xea12515 "clojure.core"],
 :name int,
 :file "clojure/core.clj",
 :column 1,
 :line 882,
 :arglists ([x]),
 :doc "Coerce to int",
 :inline
 #object[clojure.core$int__inliner__5509 0x3c79d89 "clojure.core$int__inliner__5509@3c79d89"]}

user=>
user> (set! nrepl.middleware.print/*print-fn* (fn [value writer]
                                                (some-pprint-lib/pprint value {:some-opt :foo
                                                                               :writer writer})))

This means it's possible to customise the printing without needing any client support. Any options the client does pass in the request will be preferred over the dynamic vars. For example if a client knows it cannot support the streaming mode, it can force ::print/stream? to false.

How this is implemented is by allowing any of the printing options to be specified in either the request or the response, and including the bound values of the configuration vars in eval responses. Note that this means ::print/keys can now be provided in requests – so a client that requires no pretty printing at all (e.g. using EDN transport) could send a request containing {::print/keys []}.

I have added a new section in usage/misc.adoc to document this, including the above examples.

Also fixed a couple more missing doruns in tests that were causing intermittent failures.

@cichli cichli force-pushed the cichli:streamed-printing branch 5 times, most recently from 7ff1679 to 203e2c9 Jan 26, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 27, 2019

Awesome idea! I love it!

@cichli

This comment has been minimized.

Copy link
Member Author

cichli commented Jan 27, 2019

It's used in similar contexts in filesystems and networking to denote a fixed limit in bytes... I did take care to note the unit of measurement in the docstring of the var.

It's definitely useful to tweak if you're working at the REPL with data large enough to lock it up if you accidentally print it (e.g. CLJS compiler state) – see the discussion in #117 – but (for now at least) I think the best default is to not set it.

@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented Jan 27, 2019

Oh I somehow missed the discussion, yeah then that makes sense, thanks for pointing it out.

@cichli cichli force-pushed the cichli:streamed-printing branch from 203e2c9 to 6fb5bf9 Jan 29, 2019

cichli added some commits Jan 9, 2019

Rebind *out*/*err* on every eval
Previously, these were set up just once when the session was initialised. This
meant that:

1) The semantics of the out-limit option were strange – rather than just
applying to the current message, it would modify the session, and also apply to
all subsequent messages.

2) The PrintWriter implementation relied on internals of the eval middleware to
determine on which transport to send data, making it hard to reuse without also
being aware of said internals.

Further, session-out was private, discouraging its use by other
middlewares (and, unfortunately, encouraging said middlewares to instead try to
reuse the already-created *out*/*err* on the session).

We now provide a function replying-PrintWriter which is similar to session-out
but instead uses _only_ the transport of the message it is provided. Hence, we
now must call it for every eval message, each time creating new writers which
correlate to said messages :id. Additionally, the custom buffering
implementation has been replaced with a BufferedWriter.

replying-PrintWriter is intended to be used by other middlewares that need to
stream printed data back to clients. Generally, they should not use :out or :err
as the key of their printed responses, so as to avoid the unwanted interleaving
of different kinds of printed output.

@cichli cichli force-pushed the cichli:streamed-printing branch from 6fb5bf9 to 7803165 Jan 29, 2019

cichli added some commits Jan 18, 2019

Add nrepl.middleware.print
Revert "Extend the pprint documentation"

This reverts commit b4de7a7.

Revert "Support single-arity printer functions"

This reverts commit 7200f8e.

@cichli cichli force-pushed the cichli:streamed-printing branch from 7803165 to 2ddf536 Jan 29, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 29, 2019

@cichli I guess we can merge this PR at this point. Once again - amazing work! 🙇

@cichli cichli merged commit e982a8a into nrepl:master Jan 29, 2019

3 checks passed

codecov/patch 95% of diff hit (target 75.39%)
Details
codecov/project 76.85% (+1.45%) compared to 8ee6b77
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cichli cichli deleted the cichli:streamed-printing branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment