greglook / whidbey

nREPL middleware to pretty-print colored values
The Unlicense
158 stars 10 forks source link

Add support for nREPL 0.6.0 #28

Closed cichli closed 5 years ago

cichli commented 5 years ago

nREPL now has the set!-able dynamic var nrepl.middleware.print/*print-fn* so tools like Whidbey can set a session-wide default printer, but clients can still override this on a per-request basis by including the relevant option in the request.

To be able to set! this, we need to switch from using :init to :custom-init. The former is evaluated only once when the REPL server starts, but the latter is evaluated each time a new client session is created (with set!-able bindings in place).

I have left the :nrepl-context code in place so nREPL 0.5.x still works.

Additionally, because of technomancy/leiningen#878, there's a bit of a sharp edge around using :init or :custom-init – they aren't merged in the way you might expect. Instead splice any existing form into our init code and overwrite the existing configuration with ^:replace.

Fixes #27.

greglook commented 5 years ago

@cichli to be more explicit about next steps - I think this is mergeable, but I'd like to understand the answers to my two questions above. In particular, it'd be nice not to have the extra newline in the new approach, but if it turns out to be too difficult we can move forward with this.

cichli commented 5 years ago

Sorry for the delay in coming back to this @greglook. I've pushed a new version and addressed your inline comments above.

I gave this a shot with leiningen 2.9.0 and can confirm that it pretty-prints now, but I also note that it's printing an additional newline that wasn't present before. render-str was returning newline-trimmed strings, whereas pprint output has a trailing newline.

This is tricky (but not impossible) when nrepl.middleware.print/*stream?* is true, and I didn't want to introduce any difference in rendered output between setting that on or off. It's also possible that some *print-fn* adds whitespace at the end intentionally, so it might be surprising if it were all unconditionally trimmed. I wonder if this could be solved more naturally in the client? It only needs to add a newline before the prompt if the current line isn't blank.

cichli commented 5 years ago

Pushed again with a fix for find-ns pointed out by @johannesloetzsch.

greglook commented 5 years ago

I wonder if this could be solved more naturally in the client? It only needs to add a newline before the prompt if the current line isn't blank.

Yeah, this feels like the right behavior to me. I'm thinking of the behavior of shells like zsh, which will render an extra newline before the prompt when needed. For now this can ship with some extra line spacing. 🤷‍♂️

cichli commented 5 years ago

Thanks! 🙌