-
Notifications
You must be signed in to change notification settings - Fork 180
Actually fix #537 #539
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
Actually fix #537 #539
Changes from all commits
1bb9fa3
7077b0b
e6c9a6c
8edfbdf
f169b4f
14eec03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,17 +28,35 @@ | |
[middleware-var-strs] | ||
(into [] mw-xf middleware-var-strs)) | ||
|
||
(defn- build-handler | ||
[middleware] | ||
(apply nrepl.server/default-handler (->mw-list middleware))) | ||
|
||
(defn start-nrepl | ||
[opts] | ||
(let [{:keys [handler middleware bind port] :or {port 0 bind "::"}} opts | ||
"Starts a socket-based nREPL server. Accepts a map with the following keys: | ||
|
||
* :port — defaults to 0, which autoselects an open port | ||
|
||
* :bind — bind address, by default \"::\" (falling back to \"localhost\" if | ||
\"::\" isn't resolved by the underlying network stack) | ||
|
||
* :handler — the nREPL message handler to use for each incoming connection; | ||
defaults to the result of `(nrepl.server/default-handler)` | ||
|
||
handler (cond-> (or handler nrepl.server/default-handler) | ||
middleware (apply (->mw-list middleware))) | ||
* :middleware - a sequence of vars or string which can be resolved to vars, | ||
representing middleware you wish to mix in to the nREPL handler. Vars can | ||
resolve to a sequence of vars, in which case they'll be flattened into the | ||
list of middleware." | ||
[{:keys [handler middleware bind port] :as opts}] | ||
(let [handler | ||
(if handler | ||
(handler) | ||
(build-handler middleware)) | ||
|
||
{:keys [server-socket port] :as server} | ||
(nrepl.server/start-server :handler handler | ||
:bind bind | ||
:port port) | ||
:bind (or bind "localhost") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't bind work with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I think what will happen here is an exception when this goes through travis. From
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I should clarify: I made this change after experiencing the exception in travis. Not a thought experiment, but an observation :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I was just wondering if this won't message up something for people who have disabled IPv4. Probably not a big deal now, I'll fixed this once we finally move the project to https://github.com/nrepl/nREPL There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (tools.nrepl is practically dead, so we've migrated the project out of clojure contrib to revive it) |
||
:port (or port 0)) | ||
|
||
bind | ||
(-> server-socket (.getInetAddress) (.getHostName))] | ||
|
@@ -50,8 +68,8 @@ | |
|
||
(defn init | ||
([] | ||
(init nil)) | ||
([handlers] | ||
(start-nrepl {:handler handlers}) | ||
(start-nrepl {})) | ||
([middleware] | ||
(start-nrepl {:middleware middleware}) | ||
;; Return nil so the value doesn't print | ||
nil)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
(ns cider.nrepl.main-test | ||
(:require [cider.nrepl :refer [wrap-debug cider-middleware]] | ||
[cider-nrepl.main :as m] | ||
[clojure.test :refer :all] | ||
[clojure.tools.nrepl :as nrepl] | ||
[clojure.tools.nrepl.server :as nrepl.server] | ||
[clojure.tools.nrepl.transport :as transport])) | ||
|
||
(defn start-stop-nrepl-session [opts] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see where you stop the server in each test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding was that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok. |
||
(with-open [server (#'m/start-nrepl opts) | ||
transport (nrepl/connect :port (:port server))] | ||
(transport/send transport {:op "clone" :id 1}) | ||
(let [session-id (:new-session (transport/recv transport 1000))] | ||
(assert session-id) | ||
(transport/send transport {:session session-id | ||
:id 2 | ||
:op "clone"}) | ||
(is (= (:status (transport/recv transport 1000)) ["done"]))))) | ||
|
||
(deftest start-nrepl-test | ||
(testing "passing a specific handler should work" | ||
(let [opts {:handler nrepl.server/default-handler}] | ||
(start-stop-nrepl-session opts))) | ||
|
||
(testing "passing a sequence instead of a map shouldn't crash" | ||
(let [opts ["cider.nrepl/cider-middleware"]] | ||
(start-stop-nrepl-session opts))) | ||
|
||
(testing "passing nil shouldn't crash" | ||
(let [opts nil] | ||
(start-stop-nrepl-session opts))) | ||
|
||
(testing "passing valid middleware should work" | ||
(let [opts {:middleware ["cider.nrepl/cider-middleware"]}] | ||
(start-stop-nrepl-session opts))) | ||
|
||
(testing "passing options as given by boot task middleware should work" | ||
(let [opts {:middleware '(cider.nrepl.middleware.version/wrap-version | ||
cider.nrepl.middleware.apropos/wrap-apropos) | ||
:port nil | ||
:bind nil}] | ||
(start-stop-nrepl-session opts)))) | ||
|
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 that build handler should just take a list of middlewares and be invoked only if a handler wasn't passed to
start-nrepl
.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 see, so that implies that either a handler or middleware is expected, but not both. I can make that change.
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. Normally you'd just use a predefined handler, but in some cases you might opt to build the handler yourself by supplying some list of middlewares instead.
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.
(e.g. because some middleware is problematic in your setup or something like this)