Skip to content
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 a simple nrepl-server task in cider.tasks #532

Merged
merged 3 commits into from
Jun 16, 2018

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented May 23, 2018

First step towards the new cider nrepl integration.

Before submitting a PR make sure the following things have been done:

  • The commits are consistent with our contribution guidelines
  • You've updated the README (if adding/changing middleware)

@codecov
Copy link

codecov bot commented May 23, 2018

Codecov Report

Merging #532 into master will decrease coverage by 0.46%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage   72.95%   72.48%   -0.47%     
==========================================
  Files          36       36              
  Lines        2281     2297      +16     
  Branches      134      134              
==========================================
+ Hits         1664     1665       +1     
- Misses        483      498      +15     
  Partials      134      134
Impacted Files Coverage Δ
src/cider/tasks.clj 5.55% <0%> (-5.56%) ⬇️
src/cider_nrepl/main.clj 20% <7.69%> (-1.74%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb2b5a2...fd19ad4. Read the comment docs.

@bbatsov
Copy link
Member

bbatsov commented May 26, 2018

That looks simple enough!

@arichiardi
Copy link
Contributor Author

arichiardi commented May 28, 2018

If I understand correctly - if this is what we want for boot - we basically can handle the cider-boot-parameters in cider directly.

If cider-jack-in-command sees that the project depends on:

  • [org.clojure/tools.nrepl "0.2.13"] -> "repl -s -H :: wait" (current behavior)
  • [nrepl "0.3.1"] -> "repl -s -H :: wait" (current behavior)
  • [nrepl "0.4.0"] -> "nrepl-server wait"

Problem is cider-boot-parameters is a defcustom so we probably need to take away the repl part because it suddenly becomes dynamic.

Another approach would be to always call nrepl-server:

(defcustom cider-boot-parameters
  "nrepl-server -s -H :: wait"
  "Params passed to boot to start an nREPL server via `cider-jack-in'."
  :type 'string
  :group 'cider
  :safe #'stringp
  :package-version '(cider . "0.9.0"))

And detect the nrepl version in the task.

EDIT: I like the latter better but it also means the we need to replicate the options we are using / are commonly used in reply - server. In particular the two above are:

-s, --server      Start REPL server only.                         [we get this]
-H, --host HOST   HOST sets the host client connects to.

Which consequently means we need to modify this. Writing down this things here for future reference.

@bbatsov
Copy link
Member

bbatsov commented Jun 14, 2018

CIDER won't be able to work with both versions of nREPL anyways, so there's no need to complicate this. I'm ok with the second option.

@arichiardi arichiardi changed the title WIP - Add a simple nrepl-server task in cider.tasks Add a simple nrepl-server task in cider.tasks Jun 15, 2018
It forwards to cider.nrepl.main/init passing options -b|--bind and -p|--port.
@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 15, 2018

Ok so the task accepts -p|--port and -b|--bind like boot's task so we can now do this:

nrepl-server -b :: wait

The -b to localhost is the default too.

@arichiardi
Copy link
Contributor Author

I also made the whole thing a bit more robust - we should probably merge nrepl/nrepl#34 before this just to make sure things are ok.

@arichiardi
Copy link
Contributor Author

The error in the tests is unrelated to the change. It is a zip error.

@bbatsov bbatsov merged commit 4a6be9f into clojure-emacs:master Jun 16, 2018
@bbatsov
Copy link
Member

bbatsov commented Jun 16, 2018

Nice!

Now we just need the same time for lein and then we can update the the cider-jack-in commands and finally switch to our new nREPL.

@arichiardi arichiardi deleted the nrepl-server branch June 18, 2018 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants