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

Rich printing #106

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@cgrand
Copy link
Contributor

cgrand commented Dec 17, 2018

Fix #92.

Enable rich printing by setting :printer to nrepl.elisions/printer in an :eval message.

Long/infinite data, MIME attachments and datafy/nav are supported through elisions and custom literals.

Pretty printing has to be done on the client side.

cgrand added some commits Dec 7, 2018

@cgrand cgrand requested a review from bbatsov Dec 17, 2018

@@ -0,0 +1,44 @@
(ns nrepl.elisions

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

I wonder if we shouldn't put this namespace and rich-printer under something like nrepl.print or whatever. Might be useful to structure the code like this if we ever come up with new printers.

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Some short summary of the purpose of the ns and add :added 0.6 metadata would be a nice addition as well.

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Btw, why is this a separate ns to begin with? I wonder if it wouldn't better to have some soft-store ns and everything else to be in the rich-print ns.

This comment has been minimized.

@cgrand

cgrand Dec 21, 2018

Author Contributor

elisions contains the most specific parts: the printer function (for pr-values) and the elision store strategy.

nrepl.print.elisions and nrepl.print.elisions.rich-printer?

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Yeah, that sounds good to me.

[clojure.edn :as edn]
[clojure.main :as main]))

; even with limits on breadth and depth of printing, we can easily get big values:

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

I think this big comment can be converted to a docstring.

(defprotocol MachinePrintable
(-print-on [x write rem-depth]))

;; clojure 1.10 support

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Or datafy/nav support? :-) Probably this will be a better description once 1.11 is out. :-)

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Dec 21, 2018

Apart from the build failure my general feedback is:

  • it'd be nice if the new printer had some unit tests, as I don't think one integration test is enough to cover well that it's working properly
  • All public functions should ideally have some docstring.
  • We need to add a bit of documentation to the manual about tool writers and how they can leverage the new printer.

Apart from this - the code looks very solid!

(@sessions session)
(create-session transport))]
(let [msg (-> msg (assoc :session the-session) (assoc-in [:print-options :nrepl/session] the-session))]
;; TODO: yak, this is ugly; need to cleanly thread out-limit through to

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Btw, do we still need this in light of the recent changes you've made?

(write "]"))))))

(defn base64-encode [^java.io.InputStream in]
(let [table "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Shouldn't this be some constant?

(defn base64-encode [^java.io.InputStream in]
(let [table "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"
sb (StringBuilder.)]
(loop [shift 4 buf 0]

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

There a few magic numbers if this function - I wonder if we shouldn't give them some names. For me it's hard to figure out why those values are the ones needed.

(seq x)
(str x)))})

; datafy and friends shoud be but to good use there

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Top-level comments are supposed to have 2+ ;. At least according to common Lisp lore. :-)


(defn StackTraceElement->vec'
"Constructs a data representation for a StackTraceElement"
{:added "1.9"}

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

I don't think we should copy the metadata as well. :-)

This comment has been minimized.

@bbatsov

bbatsov Dec 21, 2018

Contributor

Should those backports be private?

(ns nrepl.rich-printer
"A printer that produces a pure EDN representation of a Clojure value.
Features elisions, MIME attachments and general purpose object browsing.
Initially developed for unrepl."

This comment has been minimized.

@arichiardi

arichiardi Dec 21, 2018

Contributor

Awesome!

@cgrand

This comment has been minimized.

Copy link
Contributor Author

cgrand commented Dec 21, 2018

@bbatsov:

Apart from the build failure

Do you have any idea why Eastwood does not exit? That's what makes the build to fail.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Dec 21, 2018

Do you have any idea why Eastwood does not exit? That's what makes the build to fail.

Likely you've hit some bug in it. Does it hang locally as well? Perhaps @slipset might be able to help debugging this?

@slipset

This comment has been minimized.

Copy link
Contributor

slipset commented Dec 21, 2018

I've reproduced this with the version of eastwood you're running (0.2.6), but cannot reproduce with current eastwood (0.3.4). 0.3.4 has a new linter which catches another error. I'll create a PR with upgraded eastwood and a fix for the error.

@slipset

This comment has been minimized.

Copy link
Contributor

slipset commented Dec 21, 2018

PR #110 should fix this problem

cgrand and others added some commits Jan 9, 2019

@cgrand

This comment has been minimized.

Copy link
Contributor Author

cgrand commented Jan 9, 2019

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jan 9, 2019

I'll restart the build. If it hangs again I guess we'll have to disable it until @slipset can help us figuring out what's wrong.

@cichli

This comment has been minimized.

Copy link
Member

cichli commented Jan 19, 2019

Can the eliding phase be separated from the printing phase? So that, for example, other middleware/ops could inspect and use the elided value? Or a custom pretty printer could be used?

@bbatsov bbatsov referenced this pull request Jan 23, 2019

Merged

New print middleware #118

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