Add and populate :host and :socket keys on nrepl.server.Server #311
Conversation
|
@rlbdv Feel free to take a look as well. |
|
Can you rebase on top of |
This test currently passes a map of its own construction to run-repl, so it's not testing the return value from start-server, and doesn't catch the issue described at nrepl#309 .
This fixes an issue that occurs when using the `--interactive` option in concert with `--socket PATH`. Since the Server object has no :socket key, nrepl.cmdline/interactive-repl won't recognize that it should connect to the socket and will exit with an error message. This also ensures that consumers of the record can obtain the passed-in host value.
chancerussell
force-pushed
the
fix-interactive-with-socket-file
branch
from
f860b9f
to
d625f01
Compare
Yep, just rebased! |
src/clojure/nrepl/server.clj
Outdated
| @@ -158,7 +158,7 @@ | |||
| (binding [dynamic-loader/*state* state] | |||
| ((:handler @state) msg))))) | |||
|
|
|||
| (defrecord Server [server-socket port open-transports transport greeting handler] | |||
| (defrecord Server [server-socket host port socket open-transports transport greeting handler] | |||
There was a problem hiding this comment.
I'm wondering if here we shouldn't name this unix-socket, to differentiate from the server-socket. Not a big deal, but it might make things clearer for someone.
Or maybe we should add some docstring to this record to explain its purpose.
There was a problem hiding this comment.
I will:
- Add some explanatory comments to the
defrecordform re: what the fields are supposed to mean (unfortunately, this won't be a docstring, sincedefrecrorddoesn't support them) - Open an additional PR just like this one, but with the
socketfield renamed tounix-socket
And then I'll leave it to you all to choose which one you like better and let you update the comments as you wish :)
chancerussell
force-pushed
the
fix-interactive-with-socket-file
branch
from
2ea884f
to
5b67738
Compare
|
Can you, please, address the small indentation error detected by the lint job? |
chancerussell
force-pushed
the
fix-interactive-with-socket-file
branch
from
5b67738
to
ee97f1f
Compare
chancerussell
force-pushed
the
fix-interactive-with-socket-file
branch
from
ee97f1f
to
3986637
Compare
|
I just opened #312, which is the same as this PR, but renames the :socket field to :unix-socket and updates the bits that look at that field to account for the change. Personally, I would lean towards not renaming the key, since anyone providing their own custom repl-fn might have written it to expect a server object bearing the (I also sort of want to say the key might be better named unix-socket-path or filesystem-socket-path, since it's not holding an actual socket object, but I could bikeshed on that for days!) I'll be unavailable for a few days—please feel 100% free to make any desired changes you'd like against either PR to get this merged! |
Same. I was thinking more along the lines of duplicating the key and deprecating the old name, but it's not a big deal. I think this PR is enough for now. |
Like #310, but also provides the
:hostkey.This fixes an issue that occurs when using the
--interactiveoption in concert with--socket PATH. Since the Server object has no :socket key, nrepl.cmdline/interactive-repl won't recognize that it should connect to the socket and will exit with an error message.Also adjusts an existing test to exercise the code path in question.
Before submitting a PR make sure the following things have been done:
Thanks!
If you're just starting out to hack on nREPL you might find this section of its
manual extremely useful.