Update to nREPL 0.6.0 / cider-nrepl 0.21.0 #2579
Conversation
We now no longer need to either manually fix up the input/prompt start markers, or manually fix up the point after emitting the prompt.
Show the prefix when we emit any printed result. Don't show the prefix when we emit rich content for a specific content-type (i.e. images).
The behaviour of setting cider-repl-scroll-on-output to t is the same as setting scroll-conservatively to 101. Document this, and remove both the option and its associated logic. This results in a dramatic performance improvement in the REPL.
New option, cider-repl-init-code, is a list of strings of Clojure code to evaluate when starting a REPL (with bindings for set!-able vars in place). Options cider-repl-print-length and cider-repl-print-level are now obsolete. Set the initial namespace of the REPL using the :ns response slot of an empty eval message, rather than evaluating (str *ns*). The only other request now made during init is to evaluate cider-repl-init-code.
* New options: ** cider-print-fn ** cider-print-options ** cider-print-quota * Options made obsolete: ** cider-pprint-fn ** cider-pprint-options ** cider-stacktrace-print-options ** cider-debug-print-options ** cider-repl-pretty-print-width * Add functions to construct requests for either pretty-printing or printing with clojure.core/pr, and use these functions where relevant. * Use map.el functions for merging partial requests. * Prefer any width value in cider-print-options to the advised value provided by fill-column, to allow overriding this option without needing a separate variable.
Profiling shows that creating a new buffer each time is not free. It is also unnecessary. Reuse the same named buffer, clearing it each time.
Profiling shows we end up performing a lot of allocations in this section of code. Minimise string allocations by combining the two platform-specific newline regexps into one.
The printed value is in the value slot, not the out or err slots. Accumulate the result in the value handler and insert the actual comment in the done handler.
|
Wow! That’s one truly epic PR! Fantastic work! 🙇 I don’t see any obviously problems, so you can go ahead with merging this, so we can see if the snapshot will report any problems. |
|
I’ve just invited him to the org. |
There was a problem hiding this comment.
There's lots of things in here that look great and we are up against a hard timeframe now that lein 2.9.0 is out.
I wish there was more time to go over this but nothing jumps out at me, not that I've given it anything beyond a quick look.
I'm using it at work right now so things seem to be inorder from that perspective. I guess we can merge this and sort out any issues later. There seem to be no big blockers.
I'm a little worried about dragging people from leins 2.8.1 -> 2.8.3 -> 2.9.0 and each one doesn't play nicely with certain dependencies. At work it took us some work to be able to move from lein 2.7.1 (!).
|
I hadn’t noticed the release of Lein 2.9.0 - it’s pretty cool it uses nREPL 0.6 by default! In general I agree with your assessment - if we are certain that the core functionality works than it’s fine to merge this, wait until the end of the week for user feedback and cut a stable release then. I’m reasonably certain most users won’t noticed the printing changes, so we should probably do some promotional work at some point - blog posts, etc. I certainly intend to write something about nREPL 0.6 when I’m back home. We should probably add some upgrade notes somewhere (e.g. a plain file or something in the manual), as users are often complaining that it’s not clear what are they supposed to do when upgrading CIDER. But we can also not do it, as the current strategy seems to work, despite its flaws. 😃 |
|
Thanks for the updates. I bumped into |
|
Will test this out as well from master, thanks! Shadow-cljs user here and there was a bug with pretty printing that this might have solved. |
Please see the commit messages for more detail, but find a summary of the
changes below:
The printing config is now controlled by three options:
cider-print-fn,cider-print-options, andcider-print-quota. Now-redundant printing optionshave been made obsolete.
There are now functions for constructing partial requests for both the
pretty-print and non-pretty-print case – we need to ensure we force
clojure.core/prprinting for all the non-pprintcommands.cider-print-optionscan now be any map representation thatmap.elaccepts(seeing as we support Emacs 25+ now). We use
map-mergeto combine thepartial requests that specify the printing config.
map.elhas been updated in ELPA to support plists. Once it has a newrelease we can depend on, we could change
nrepl-send-requestto accept anymap representation as its
requestargument. There are probably many otherplaces where we use ad-hoc representations of maps that we should change to
use
map.el.Some general enhancements to the REPL experience, mostly focused on performance:
Simplify insertion of images so we don't need to manually fix up any markers
or the point.
New option
cider-repl-init-codereplacescider-repl-print-lengthandcider-repl-print-level. This offers a more general way to configurablyset!any dynamic vars or do anything else needed to prep the REPLenvironment.
Remove redundant
cider-scroll-on-outputoption (and its associated customrecentering logic) and document how to restore the old behaviour by setting
scroll-conservatively.I spent some time profiling printing of some large results at the REPL and
found a couple of easy wins around excessive object allocation (we spend a
surprising amount of time in GC when the REPL is printing):
Reuse the same buffer in
nrepl-bdecoderather than creating a new oneeach time.
Combine calls to
replace-regexp-in-string.Ensure we always insert output before the REPL prompt.
Also fixed a bug causing
cider-pprint-eval-last-sexp-to-commentandcider-pprint-eval-defun-to-commentto not insert anything.