add pprint

[kolharsam]

WIP...to add the pprint option.

[borkdude]

@kolharsam To be honest, I'm not sure what should happen, so I'm going to ask @bbatsov what is the idea of #18. Are clients going to pass in an option that they want to get results printed in a certain way?

[kolharsam]

@borkdude I thought so since bb has a nrepl-server flag and you could provide options there and this would/can be one of them too.

[borkdude]

@kolharsam @bbatsov How does this work with nREPL on the JVM? Do you have an example of how this works over there?

[bbatsov]

Keep in mind that the print functionality in nREPL is generic, meaning that pprint is just one possible printer, not some special case that has to be handled separately. The implementation here definitely seems to differ from what nREPL does. In your case probably you want to have eval support any print function or something along those lines.

[bbatsov]

It's described here in great detail https://nrepl.org/nrepl/0.8/design/middleware.html#pretty-printing

Basically, the expectation is the desired printer function is available on the classpath (it will be auto-required if so).

[borkdude]

@bbatsov Thanks! Yeah, those docs are pretty great. What I meant was: are these also user-facing options? Or does lein repl :connect ... e.g. decide for itself if it wants to invoke these middleware options?

[bbatsov]

Currently they are only meant to be passed via requests. Eventually there might be some defaults passed via the config/command-line, but that's not currently the case.

[borkdude]

@bbatsov So if I understand correctly, when a user invokes cider-pprint-eval-last-sexp then CIDER will wrap the middleware in the request, at least this is what I'm seeing here, sort of in cider-client.el:

(map-merge 'list
               `(("nrepl.middleware.print/stream?" "1"))
               (when cider-print-fn
                 `(("nrepl.middleware.print/print" ,(cider--print-fn))))
               (when cider-print-quota
                 `(("nrepl.middleware.print/quota" ,cider-print-quota)))
               (when cider-print-buffer-size
                 `(("nrepl.middleware.print/buffer-size" ,cider-print-buffer-size)))
               (unless (nrepl-dict-empty-p print-options)
                 `(("nrepl.middleware.print/options" ,print-options))))

So the babashka nrepl just has to check for this value and then do a lookup on supported print-functions, etc. It's starting to make sense I think.

[kolharsam]

@borkdude are you suggesting that we do something similar here as well? That I feel would be ideal and a lot simpler to do.

[bbatsov]

then CIDER will wrap the middleware in the request, at least this is what I'm seeing here, sort of in cider-client.el

More like add those params to the eval request, as there's no way to invoke directly the print middleware in nREPL - it's meant to automatically process the output of other middleware like eval, that depend on print. That's why I said in your design it'd be best to just have the print functionality be part of eval directly.

So the babashka nrepl just has to check for this value and then do a lookup on supported print-functions

Yeah, more or less, that's what's needed.

@borkdude are you suggesting that we do something similar here as well? That I feel would be ideal and a lot simpler to do.

It has to be similar at the request/response API level, otherwise babashka's nREPL won't work properly with existing Clojure clients.

[borkdude]

@kolharsam So to re-iterate. We should handle this within eval-msg. That function should not get an extra parameter, but we should inspect the message on the presence of a print middleware key, near here:

babashka.nrepl/src/babashka/nrepl/impl/server.clj

Line 23 in 8830a50

file (:file msg)]

If the middleware key is present, we should resolve that function. I suggest that we do not use Clojure's resolve for this, since that doesn't work well with GraalVM: it bloats the binary and since babashka.nrepl is used within native-images, you can't dynamically add libraries anyway. Maybe we can hardcode a map for now in which we look up the print functions and currently only support clojure.core/prn and clojure.core/pprint? Any thoughts on this @bbatsov?

If the middleware key is not present we default to pr-str (current behavior).

The printing happens here:

babashka.nrepl/src/babashka/nrepl/impl/server.clj

Line 49 in 8830a50

"value" (pr-str value)}) opts)

[bbatsov]

If the middleware key is present, we should resolve that function. I suggest that we do not use Clojure's resolve for this, since that doesn't work well with GraalVM: it bloats the binary and since babashka.nrepl is used within native-images, you can't dynamically add libraries anyway. Maybe we can hardcode a map for now in which we look up the print functions and currently only support clojure.core/prn and clojure.core/pprint? Any thoughts on this @bbatsov?

Sounds very reasonable to me.

[kolharsam]

Thanks for breaking it down @borkdude and thanks to @bbatsov as well. I guess this is a lot clearer. Should be able to get it to work

[kolharsam]

@borkdude I have updated the PR; I'm not sure if I was able to follow your remarks correctly but please let me know how I can improve it.

[borkdude]

@kolharsam I added three comments.

[kolharsam]

@borkdude I have updated the PR as per the previous review.
Also, are the tests failing because of a missing require?

[borkdude]

I inspected the nREPL messages when calling cider-pprint-eval-last-sexpr in emacs for (+ 1 2 3) and this printed:

  op                                 "eval"
  session                            "38c35386-c53f-4338-9b8f-db523fee808d"
  time-stamp                         "2020-12-08 12:48:31.682727000"
  code                               "(+ 1 2 3)
"
  column                             1
  file                               "/Users/borkdude/dre/DocSearch/app/src/dre/article/search.clj"
  line                               48
  nrepl.middleware.print/buffer-size 4096
  nrepl.middleware.print/options     (dict ...)
  nrepl.middleware.print/print       "cider.nrepl.pprint/pprint"
  nrepl.middleware.print/quota       1048576
  nrepl.middleware.print/stream?     "1"
  ns                                 #("dre.article.search" 0 18 (fontified t help-echo cider--help-echo cider-locals nil cider-block-dynamic-font-lock t face font-lock-type-face))
)
(<--
  id         "26"
  session    "38c35386-c53f-4338-9b8f-db523fee808d"
  time-stamp "2020-12-08 12:48:31.735272000"
  value      "6"
)
(<--
  id         "26"
  session    "38c35386-c53f-4338-9b8f-db523fee808d"
  time-stamp "2020-12-08 12:48:31.735588000"
  ns         "dre.article.search"
)
(<--
  id         "26"
  session    "38c35386-c53f-4338-9b8f-db523fee808d"
  time-stamp "2020-12-08 12:48:31.735800000"
  status     ("done")
)

So the pprint function here is cider.nrepl.pprint/pprint (which seems to be a simple wrapper around clojure pprint: https://github.com/clojure-emacs/cider-nrepl/blob/94d6c9ee020b9d78e0ee68a60a9e5cb854414352/src/cider/nrepl/pprint.clj#L43). Should babashka be able to handle this or should CIDER have some way of asking for the available pprint functions and then invoke the preferred one @bbatsov?

[borkdude]

@kolharsam The tests are failing because you have to require 'clojure.pprint` first.

Syntax error (ClassNotFoundException) compiling at (babashka/nrepl/impl/server.clj:17:1).
clojure.pprint

[kolharsam]

@borkdude should I be updating any tests for this?

[borkdude]

@kolharsam Yes, a test for this would be nice, but still waiting on @bbatsov input on:

#33 (comment)

We don't know about a cider.nrepl.pprint/pprint function in babashka.nrepl and given that emacs / CIDER sends this on cider-pprint-eval-last-sexpr, then the babashka nREPL pprinting still wouldn't work. So let's sort this out first.

[bbatsov]

Should babashka be able to handle this or should CIDER have some way of asking for the available pprint functions and then invoke the preferred one @bbatsov?

That would certainly be convenient for the users, but it's totally up to you. We can also ask them to configure the appropriate function manually themselves in CIDER's docs. Less configuration, doesn't hurt though. :-)

[borkdude]

@bbatsov So for convenience it would be ok to map cider.nrepl.pprint/pprint to clojure.pprint/pprint right?

[bbatsov]

Yep. The main reason for the wrapper is to expose the supported options via a parameter, as pprint doesn't support them in this fashion (only via dynamic vars).

[borkdude]

@bbatsov In this case it seems better to just copy over the cider pprint function and support also the nrepl.middleware.print/options value on this.

[borkdude]

@kolharsam Note that clojure.pprint/pprint prints to *out*. To return a string we'll need to use with-out-str here.

[borkdude]

@Grazfather has offered to work on pt. 2 in a new PR to the pprint branch. Thanks so far @kolharsam