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

Introduce external server configuration #66

Merged
merged 3 commits into from Nov 2, 2018

Conversation

Projects
None yet
4 participants
@bbatsov
Copy link
Contributor

bbatsov commented Oct 28, 2018

That's something I've been thinking about for a long time and it's finally coming in 0.5 (after a polish it and add some docs).

Basically the idea is to make it easier to have host-wide and project-specific configs, so you can have stable bind-addresses, ports, transports, middleware, etc.

Obviously that's also going to require a bit of changes to tools like boot and lein, but those are going to be trivial. Feedback is welcome!

I also plan to follow this up with clj writing in .nrepl/ some information about each running server, so it's easy for something like CIDER to just go check this dir and suggest all the local running instances as something you can connect to. That's going to be a massive improvement over the ps grepping we do right now and will finally work on Windows as well. :-)

(catch java.io.IOException e
(printf "Couldn't open '%s': %s\n" source (.getMessage e)))
(catch RuntimeException e
(printf "Error parsing edn file '%s': %s\n" source (.getMessage e)))))

This comment has been minimized.

@isker

isker Oct 30, 2018

Why do we catch these exceptions at all?

This comment has been minimized.

@bbatsov

bbatsov Oct 30, 2018

Author Contributor

Good point! I was just playing with some things here. I guess I should just clean this up and silently return nil or {}.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 30, 2018

Codecov Report

Merging #66 into master will decrease coverage by 1.59%.
The diff coverage is 28.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #66     +/-   ##
=========================================
- Coverage   79.44%   77.85%   -1.6%     
=========================================
  Files          12       13      +1     
  Lines         978     1025     +47     
  Branches       33       34      +1     
=========================================
+ Hits          777      798     +21     
- Misses        168      193     +25     
- Partials       33       34      +1
Impacted Files Coverage Δ
src/clojure/nrepl/server.clj 87.5% <0%> (ø) ⬆️
src/clojure/nrepl/cmdline.clj 22.22% <8.16%> (-2.03%) ⬇️
src/clojure/nrepl/config.clj 80% <80%> (ø)
...rc/clojure/nrepl/middleware/interruptible_eval.clj 95.73% <0%> (+0.6%) ⬆️

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 36de208...56f2200. Read the comment docs.

@isker

This comment has been minimized.

Copy link

isker commented Oct 30, 2018

Looks convincing to me, though this is my first time reading nrepl code 👍.

@bbatsov bbatsov force-pushed the server-config branch 2 times, most recently from 58c1a0a to cd64a3f Oct 30, 2018

bbatsov added some commits Oct 27, 2018

[#56] Bind the server to 127.0.0.1 by default
This avoid a dependency on the order of the localhost records in
/etc/hosts. Normally first there's a record for 127.0.0.1 (ipv4
loopback address) and then there's a record for ::1 (ipv6 loopback
address), so normally you'd end up using the ipv4 address, but that's
not guaranteed.

I'd rather us have a stable and explicit default.
Introduce the nrepl.config ns
If you have .nrepl-config.edn in the folder you invoke nREPL from
it will take precedence over whatever is in `.nrepl/config.edn`.

That might be very handy for project specific configuration - e.g.
fixed port per project, fixed transport, etc.

@bbatsov bbatsov force-pushed the server-config branch from 56f2200 to 0a6d57b Nov 2, 2018

@bbatsov bbatsov merged commit 9129bf6 into master Nov 2, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@bbatsov bbatsov deleted the server-config branch Nov 2, 2018

@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented Nov 2, 2018

Awesome PR, this is my way forward from now on thanks!

@arichiardi

This comment has been minimized.

Copy link
Contributor

arichiardi commented on doc/modules/ROOT/pages/usage/server.adoc in 0a6d57b Nov 2, 2018

Am I on time for a name suggestion?

The -config part seems a bit redundant because all the edn project files are somehow a config. Other tools also don't have the dash config: shadow-cljs.edn, figwheel-main.edn, ...

Consequently it would be a sacrilege to me to have e $HOME/.nrepl/nrepl.edn.

This comment has been minimized.

Copy link
Contributor Author

bbatsov replied Nov 2, 2018

I'm fine with dropping the config part, but I don't get the second suggestion.

This comment has been minimized.

Copy link
Contributor

arichiardi replied Nov 2, 2018

For consistency, if we drop it in one place maybe we should drop it in both and call the file nrepl.edn everywhere

This comment has been minimized.

Copy link
Contributor Author

bbatsov replied Nov 2, 2018

Consequently it would be a sacrilege to me to have e $HOME/.nrepl/nrepl.edn.

I guess you missed a not here. :D Now I see your point. There are going to be some other files in this folder (auth token files, server instances files, etc), so the naming made some sense to me, but I'll think about this.

This comment has been minimized.

Copy link
Contributor

arichiardi replied Nov 3, 2018

Ops, right! Ok thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment