-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add support for nREPL 0.6.0 #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for migrating this to use the new printing middleware. A couple comments, but the approach looks good. I like the fix for #21 as well. 👍
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.
([value writer] | ||
(render value writer nil)) | ||
([value writer opts] | ||
(binding [*out* writer] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comments you added in nrepl/nrepl#118 it seems like this is the undesirable interleaved-output pattern, right? I checked fipp briefly and it looks like usage of *out*
is pretty baked in here, so I don't know that it can be easily remedied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly. See brandonbloom/fipp#56 – that adds an explicit :writer
option.
src/whidbey/repl.clj
Outdated
@@ -78,6 +95,7 @@ | |||
(defn init! | |||
"Initializes the repl to use Whidbey's customizations." | |||
[options] | |||
(update-print-fn!) | |||
(update-options! options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is run on each new nREPL session now, does that mean that reconnecting to the repl would update your whidbey options? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't thought of this. I've pushed a new version that still uses :init
for the Whidbey options. Maybe it makes sense to change those to a set!
-able dynamic var so they can be per-session, but it needn't be in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way it is now works - I wouldn't want two users connecting to the repl to fight over whose options are in effect.
To be compliant with greglook/whidbey#28 fixes: greglook/whidbey#21
@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. |
Sorry for the delay in coming back to this @greglook. I've pushed a new version and addressed your inline comments above.
This is tricky (but not impossible) when |
* Set nrepl.middleware.print/*print-fn* accordingly if it is present. * Add a comment noting that the :printer option only applies to nREPL 0.5.x.
Leiningen doesn't know how to merge these - see technomancy/leiningen#878. Instead, use ^:replace metadata to overwrite it completely with a form that evaluates any existing value before evaluating our init code.
Pushed again with a fix for |
Yeah, this feels like the right behavior to me. I'm thinking of the behavior of shells like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Thanks! 🙌 |
nREPL now has the
set!
-able dynamic varnrepl.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 (withset!
-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.