Inject cider REPL dependencies at cider-jack-in #1552

benedekfazekas · 2016-02-09T11:45:31Z

So users don't have to fiddle with profiles.clj and the like, see
details in #1531 Supports both leiningen and boot. Additionally there is
a defcustom cider-skip-repl-dependencies-at-jack-in to switch this
functionality off (defaults to nil); and an extension point
defun (cider-add-repl-dependencies) so other tools like clj-refactor
can inject to their own dependencies, middlewares

Fix #1531 and #1534

expez · 2016-02-09T11:57:06Z

cider.el

@@ -165,6 +165,11 @@ This variable is used by `cider-connect'."
  :type 'boolean
  :version "0.9.0")

+(defcustom cider-skip-repl-dependencies-at-jack-in nil


Why use a negation here? Why not call it cider-inject-dependencies or something with a default of t?

I guess to stress the fact that we default to inject. but I am easy, happy to rename.

I agree with @expez.

bbatsov · 2016-02-09T12:07:42Z

This should also be in the README and the changelog. In particular the README's installation section will need some updating.

expez · 2016-02-09T12:11:41Z

cider.el

+(defvar cider-repl-dependencies
+  `((("org.clojure/tools.nrepl" "0.2.12"))
+    (("cider/cider-nrepl" ,(upcase cider-version)))
+    ("cider.nrepl/cider-middleware"))


I suggest we split this var in two: cider-repl-dependencies and cider-repl-middleware or something similar.

thinking to leave this as is but rather split cider-add-repl-dependencies into add-middlewares and add-dependencies

Why introduce complexity (var which holds two types of data) when simplification also makes it easier to work with?

I agree with @expez. It's confusing right now.

Oh, and we should definitely mark these variables as risky-local.

benedekfazekas · 2016-02-09T14:07:53Z

@bbatsov changelog and README is done

bbatsov · 2016-02-09T15:07:59Z

cider.el

+  "List of Clojure artifacts for starting the REPL.
+First item is a list of dependencies where elements are lists of artifact name and version.
+Second item is a list of leiningen plugins where elements are lists of artifact name and version.
+Third item is a list of nrepl middleware names.")


nrepl -> nREPL

bbatsov · 2016-02-09T16:19:35Z

Just to be clear, as it's kind of hard to figure out what the code does without tests - this simply passes more params to lein and boot, right?

I think it should also account for cider-jack-in-clojurescript as there we have additional params (e.g. piggieback).

Malabarba · 2016-02-09T16:52:12Z

cider.el

+  (concat
+   (mapconcat
+    'identity
+    (seq-map (lambda (dep) (format "-d %s:%s" (car dep) (cadr dep))) dependencies)


This seq-map can be removed by replacing that 'identity above with this (lambda ...).
Same thing for the ones below.

Malabarba · 2016-02-09T17:07:50Z

Thanks for working on this Benedek.

I agree that the functions that manipulate the parameters need tests. Right now, it's a little hard to guess what they're supposed to do, so a couple of tests for each of them are sure to be helpful in the future.

bbatsov · 2016-02-09T19:12:54Z

@benedekfazekas Btw, just to be clear - this will take precedence over something in profiles.clj, right?

benedekfazekas · 2016-02-09T19:21:38Z

Just to be clear, as it's kind of hard to figure out what the code does without tests - this simply passes more params to lein and boot, right?

if you jack in you should see what it generates to start up the repl. if there is additional params (for cljs): code only modifies params so it should be fine. but you are right I deffo need to test it.

Btw, just to be clear - this will take precedence over something in profiles.clj, right?

I assumed so but noted: need to explicitly test this too.

benedekfazekas · 2016-02-09T19:58:27Z

the documentation https://github.com/technomancy/leiningen/blob/master/doc/PROFILES.md#declaring-profiles is not decisive.

bbatsov · 2016-02-09T21:12:16Z

Guess you'll have to test it and see what happens.

benedekfazekas · 2016-02-11T22:17:23Z

thanks for the review, I think I made all the changes requested.

Tested precedence: CLI params win over ~/lein/profiles.clj.

Postponed syncing with master as master build seems to be broken atm.

benedekfazekas · 2016-02-11T22:21:05Z

it seems I have the same test failure as master. suppose I still wait until that is fixed on master...

bbatsov · 2016-02-11T22:23:23Z

Postponed syncing with master as master build seems to be broken atm.

Actually it seems the fucked up tests are broken. I'm 100% sure that the code I wrote is correct, but the I'm not nearly as sure about the odd logic of the failing 2 tests. I'll likely just delete them...

bbatsov · 2016-02-11T22:26:48Z

cider.el

@@ -177,6 +182,18 @@ Sub-match 1 must be the project path.")
 (defvar cider-host-history nil
  "Completion history for connection hosts.")

+(defvar cider-jack-in-dependencies
+  '(("org.clojure/tools.nrepl" "0.2.12"))
+  "Clojure artifacts for starting the REPL. List of dependencies where elements are lists of artifact name and version.")


All 3 docstring start in more or less the same manner. This looks like some mistake to me.

Haha that was intentional. I guess my semi-conscious attempt to group them. happy to remove the first sentence if you'd like.

bbatsov · 2016-02-12T11:24:44Z

test/cider-tests.el

+                   "-d org.clojure/tools.nrepl:0.2.12 -d cider/cider-nrepl:0.11.0-SNAPSHOT repl -m cider.nrepl/cider-middleware -s wait")))
+
+(ert-deftest cider-inject-jack-in-dependencies-add-refactor-nrepl ()
+  (setq cider-jack-in-lein-plugins (seq-concatenate 'list cider-jack-in-lein-plugins '(("refactor-nrepl" "2.0.0"))))


Use let instead of setq. Those are dynamic variables and you don't want the tests to affect the defaults.

bbatsov · 2016-02-12T11:25:26Z

test/cider-tests.el

@@ -655,3 +655,17 @@
  (should (equal (cider-project-name "") "-"))
  (should (equal (cider-project-name "path/to/project") "project"))
  (should (equal (cider-project-name "path/to/project/") "project")))
+
+(ert-deftest cider-inject-jack-in-dependencies ()
+  (should (string= (cider-inject-jack-in-dependencies "repl :headless" "lein")


I'd suggest stubbing the version and having one test for a snapshot version and one test for a stable version.

So users don't have to fiddle with profiles.clj and the like, see details in clojure-emacs#1531 Supports both leiningen and boot. Additionally there is a defcustom `cider-inject-dependencies-at-jack-in` to control this functionality (defaults to t); other tools (like clj-refactor) can modify `cider-jack-in-dependencies`, `cider-jack-in-lein-plugins` and `cider-jack-in-nrepl-middlewares` to inject their own dependencies. Fix clojure-emacs#1531, clojure-emacs#1534

Inject cider REPL dependencies at cider-jack-in

bbatsov · 2016-02-14T11:36:57Z

👍

benedekfazekas · 2016-02-14T11:39:52Z

Woot! :)

phillord · 2016-02-14T12:45:18Z

Thank you all. This will have a big impact on the usability of cider, I think.

benedekfazekas · 2016-02-14T12:48:38Z

I hope you are right. As I said on twitter thx for bringing this up.

expez reviewed Feb 9, 2016
View reviewed changes

benedekfazekas force-pushed the master branch from 7b56e5f to e28d0ad Compare February 9, 2016 14:07

bbatsov reviewed Feb 9, 2016
View reviewed changes

Malabarba reviewed Feb 9, 2016
View reviewed changes

benedekfazekas force-pushed the master branch from e28d0ad to 93b7b92 Compare February 11, 2016 22:14

bbatsov reviewed Feb 11, 2016
View reviewed changes

benedekfazekas force-pushed the master branch from 93b7b92 to 14ddb78 Compare February 12, 2016 09:40

bbatsov reviewed Feb 12, 2016
View reviewed changes

benedekfazekas force-pushed the master branch from 14ddb78 to 065973b Compare February 13, 2016 22:31

benedekfazekas force-pushed the master branch from 065973b to 2b4cf44 Compare February 13, 2016 22:40

bbatsov added a commit that referenced this pull request Feb 14, 2016

Merge pull request #1552 from benedekfazekas/master

c9e8d61

Inject cider REPL dependencies at cider-jack-in

bbatsov merged commit c9e8d61 into clojure-emacs:master Feb 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inject cider REPL dependencies at cider-jack-in #1552

Inject cider REPL dependencies at cider-jack-in #1552

benedekfazekas commented Feb 9, 2016

expez Feb 9, 2016

benedekfazekas Feb 9, 2016

bbatsov Feb 9, 2016

benedekfazekas Feb 9, 2016

bbatsov commented Feb 9, 2016

expez Feb 9, 2016

benedekfazekas Feb 9, 2016

expez Feb 9, 2016

bbatsov Feb 9, 2016

Malabarba Feb 9, 2016

Malabarba Feb 9, 2016

benedekfazekas commented Feb 9, 2016

bbatsov Feb 9, 2016

bbatsov commented Feb 9, 2016

Malabarba Feb 9, 2016

Malabarba commented Feb 9, 2016

bbatsov commented Feb 9, 2016

benedekfazekas commented Feb 9, 2016

benedekfazekas commented Feb 9, 2016

bbatsov commented Feb 9, 2016

benedekfazekas commented Feb 11, 2016

benedekfazekas commented Feb 11, 2016

bbatsov commented Feb 11, 2016

bbatsov Feb 11, 2016

benedekfazekas Feb 11, 2016

bbatsov Feb 12, 2016

bbatsov Feb 12, 2016

bbatsov commented Feb 14, 2016

benedekfazekas commented Feb 14, 2016

phillord commented Feb 14, 2016

benedekfazekas commented Feb 14, 2016

Inject cider REPL dependencies at cider-jack-in #1552

Inject cider REPL dependencies at cider-jack-in #1552

Conversation

benedekfazekas commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedekfazekas commented Feb 9, 2016

Choose a reason for hiding this comment

bbatsov commented Feb 9, 2016

Choose a reason for hiding this comment

Malabarba commented Feb 9, 2016

bbatsov commented Feb 9, 2016

benedekfazekas commented Feb 9, 2016

benedekfazekas commented Feb 9, 2016

bbatsov commented Feb 9, 2016

benedekfazekas commented Feb 11, 2016

benedekfazekas commented Feb 11, 2016

bbatsov commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Feb 14, 2016

benedekfazekas commented Feb 14, 2016

phillord commented Feb 14, 2016

benedekfazekas commented Feb 14, 2016