Custom pretty printing

[cichli]

Context: clojure-emacs/cider#1179

This PR adds a new wrap-pprint-fn middleware, that accepts the set of all printing-related options that we currently use for the eval/stacktrace/test/refresh middlewares, in addition to a new pprint-fn option (which must be the name of the function to be used for printing, e.g. "clojure.pprint/pprint"). It then constructs a closure that wraps up all the options and the provided printing function, placing it into the request map (again under the pprint-fn key) so that subsequent middlewares can just call the function without having to care about all the different options.

Eventually I'd like to provide some alternative printers to clojure.pprint/pprint out-of-the-box, but ATM the main candidates (fipp; puget; aprint) all require at least Clojure 1.6, so I'm holding off on adding them as dependencies for now.

Two places where we could be using this, but aren't yet:

• In the debugger, we call stacktrace/analyze-causes in eval-with-locals and it would be a bit awkward to pass pprint-fn all the way down the stack... @Malabarba thoughts? Just use a dynamic var maybe?
• In the macroexpansion middleware, we call clojure.pprint/write directly - I still need to investigate if it makes sense to allow use of a custom printing fn here.

Note that there's a breaking change here - the right-margin option has been renamed to print-right-margin, for consistency. cc @laurentpetit @tpope

[arrdem]

Awesome!

[Malabarba]

This PR adds a new wrap-pprint-fn middleware, [...] so that subsequent middlewares can just call the function without having to care about all the different options.

Nice! I like the idea.

Will this obsolete the current pprint middleware? Nevermind this, see my other comment.

Eventually I'd like to provide some alternative printers to clojure.pprint/pprint out-of-the-box, (fipp; puget; aprint)

One thing you could do currently is check for the presence of any of these libs, and use it if it's available.

This way the user wouldn't have to manually configure anything complicated, they would only need to add the lib to their :dev profile :dependencies.

In the debugger, we call stacktrace/analyze-causes in eval-with-locals and it would be a bit awkward to pass pprint-fn all the way down the stack... @Malabarba thoughts? Just use a dynamic var maybe?

We could use a dynamic var to store the pprint-fn from the message that instrumented the function, but it would be even easier to just get (:print-fn *msg*) at the time of printing.
This would also make more sense, I think.

Would that work?

[Malabarba]

Couldn't this middleware also take into consideration the pprint option itself?

For instance, we can:

1. Rename :pprint-fn to :print-fn
2. wrap-pprint-fn uses clojure.core/print instead of pprint/pprint if the message's pprint parameter is nil
3. All other middleware can always just use the :print-fn for printing, without worrying about deciding whether to pretty-print or not

Would this be useful?

[cichli]

One thing you could do currently is check for the presence of any of these libs, and use it if it's available.

Yeah, this is the way to go. I'll just need to check if their APIs have remained stable for the past few versions, or if we need to sniff the version that is available.

We could use a dynamic var to store the pprint-fn from the message that instrumented the function, but it would be even easier to just get (:print-fn msg) at the time of printing.
This would also make more sense, I think.

Oh, of course, the debugger relies on eval - so we know that *msg* is bound. That definitely makes more sense. Thanks!

Couldn't this middleware also take into consideration the pprint option itself? ...

Hmm, I like the idea, but I think the pprint option only really applies to eval right now - the ex-data map in the stacktrace display should probably always be pretty-printed. Let's leave it as pprint-fn for now, and possibly add a separate print-fn in the future if we need it?

One idea I did have is an op that would let you press a button while viewing a stacktrace to either pprint or pr the map into a result buffer, or onto the kill ring, for pasting into a gist / the REPL / wherever else (guess this would be handy in the inspector too). For that new op, print-fn probably makes more sense than pprint-fn.

[Malabarba]

Oh, of course, the debugger relies on eval - so we know that msg is bound. That definitely makes more sense. Thanks!

Actually, now that you mentioned it, I remember that's not always the case.
Instrumented code can be executed outside an eval op (for instance, in a jetty server).

It's not a huge deal, but it comes up a lot, and it means we can't just rely on *msg* being set. We have to either: (1) check if it's set and fallback on pr during execution; or (2) get the :pprint-fn from the *msg* and bind it in a dynamic var (this would go in the breakpoint macro, at the same place where we bind *skip-breaks* and *locals*).
(If you're not sure what I'm talking about here, I can take care of that after we merge this).

I'm fine with both options. The second would be more troublesome, but would ensure we respect pprint in more situations.

Couldn't this middleware also take into consideration the pprint option itself? ...

Hmm, I like the idea, but I think the pprint option only really applies to eval right now - the ex-data map in the stacktrace display should probably always be pretty-printed. Let's leave it as pprint-fn for now, and possibly add a separate print-fn in the future if we need it?

Makes sense. 👍

[bbatsov]

I guess this should be rebased and merged.

[bbatsov]

@cichli ping :-)

[cichli]

@bbatsov pong :-). Sorry for letting this one languish - I'm finishing off the elisp bits for this now.

[bbatsov]

Roger that. :-)

On Thursday, 3 December 2015, Michael Griffiths notifications@github.com
wrote:

@bbatsov https://github.com/bbatsov pong :-). Sorry for letting this
one languish - I'm finishing off the elisp bits for this now.

—
Reply to this email directly or view it on GitHub
#247 (comment)
.

Best Regards,
Bozhidar Batsov

http://www.batsov.com

[bbatsov]

I see some odd test failures. Did someone look into them?

[expez]

@benedekfazekas seems mranderson is failing in some of these test runs

[benedekfazekas]

ack. will check

[cichli]

Looks like all of the JDK6 builds and the Eastwood are failing on master as well as the PR branch? https://travis-ci.org/clojure-emacs/cider-nrepl/builds/94641627

I'll see if I can track down the Eastwood one after work today.

[benedekfazekas]

A possible workaround could be to tell mranderson to skip java class repackaging: lein2 source-deps :skip-javaclass-repackage true

context
cider currently does not use any dependency which has java class files in them so this feature is not really needed anyway. and it seems that sometimes (perhaps under java6?!) the zip output stream freaks out on this. tbh mranderson should nicely detect this and skip this step automatically.

[bbatsov]

tbh mranderson should nicely detect this and skip this step automatically.

Yeah, that seems like the smart thing to do.

[benedekfazekas]

yup I created an issue so I don't forget but the workaround should unblock the PR

[bbatsov]

I removed the support for Clojure 1.5 and 1.6 (and Java 6). Now you can finally wrap this PR.

[bbatsov]

Seems that some eastwood check is breaking the build.

[bbatsov]

The readme also has to be updated.

[Malabarba]

The Eastwood failure is still about the data-readers file.

[bbatsov]

We should exclude this file or something. Or ignore the eastwood failures for now.

[bbatsov]

👍