Allow predicates in plugin and middleware lists #2238

raxod502 · 2018-03-14T22:28:47Z

This pull request is a follow-up to discussion on the clj-refactor.el issue tracker. Basically, the problem is that clj-refactor.el wants to inject Leiningen plugins and nREPL middleware, but only for REPLs within a project context. (If the middleware is injected outside a project context, then errors result.) Unfortunately, CIDER provides no way to change cider-jack-in-lein-plugins or cider-jack-in-nrepl-middlewares without using advice. This pull request adds support for including arbitrary attributes in those variables using keyword arguments. In particular, the :predicate keyword allows for conditionally adding to those variables. I designed the interface with the idea of not breaking backwards compatibility and not making it hard to reverse the change later, if desired.

I haven't gone through the checklist yet, since I'd first like feedback on whether this is a reasonable approach to the problem.

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (make test)
All code passes the linter (make lint) which is based on elisp-lint and includes
- byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

bbatsov · 2018-03-15T16:18:58Z

cider.el

@@ -358,17 +358,57 @@ specifying the artifact ID, and the second element the version number."
                       (string :tag "Version"))))

 (defvar cider-jack-in-lein-plugins nil
-  "List of Leiningen plugins where elements are lists of artifact name and version.")
+  "List of Leiningen plugins to be injected at jack-in.
+Each element is a list of artifact name and version, followed optionally by


I'm reasonably certain without some examples most people would be really confused what are they supposed to put here.

@bbatsov Do you mean in terms of the format of the keyword arguments, or why you would want to use a predicate?

If the latter, I'm not sure that this is really meant to be an end-user feature. The only current application is in clj-refactor.el, after all. But I would not at all be opposed to adding examples.

I see. That makes sense, but it's exactly the kind of info that has to be documented. First I thought this might be something users would want to use in general or something.

I guess the right solution would be just add some logic that removes those plugins from the deps if they are present there and add cider-nrepl to the list of default deps where it belong. Then we stop using this variable for non lein projects.

bbatsov · 2018-03-15T16:19:19Z

cider.el

 (put 'cider-jack-in-lein-plugins 'risky-local-variable t)
 (cider-add-to-alist 'cider-jack-in-lein-plugins
                    "cider/cider-nrepl" (upcase cider-version))

+(defun cider-jack-in-lein-plugins ()


Why isn't this function used anywhere?

@bbatsov Because I forgot to update the places where cider-jack-in-lein-plugins is used. If the approach seems reasonable, I'll go and fix that. The idea is just to take any place where the variable cider-jack-in-lein-plugins is referenced, and replace the variable with a function call.

bbatsov · 2018-03-15T16:19:55Z

Btw, if you want some project-specific settings isn't .dir-locals.el the simple way to do it?

raxod502 · 2018-03-15T21:09:42Z

@bbatsov Yes, but that doesn't work in the case of clj-refactor.el, since a solution using directory-local variables would mean that all users who wish to use clj-refactor.el are required to create a .dir-locals.el file specifying the internal middleware and Leiningen plugins used by clj-refactor.el, in every one of their projects. That would be a terrible user experience.

raxod502 · 2018-03-18T18:23:24Z

@bbatsov Aside from your comments, does this approach seem reasonable? If so, I'll go ahead and fix the remaining technical problems with this PR.

bbatsov · 2018-03-19T04:22:20Z

Yeah, this seems reasonable.

xiongtx · 2018-03-19T04:37:26Z

cider.el

+  "Return a normalized list of Leiningen plugins to be injected.
+See `cider-jack-in-lein-plugins' for the format, except that the list
+returned by this function does not include keyword arguments."
+  (mapcar


This seem like it can be made more readable (i.e. linear) w/ a thread-last.

@xiongtx Agreed, fixed.

xiongtx · 2018-03-19T04:38:28Z

cider.el

+returned by this function only contains strings."
+  (mapcar
+   (lambda (spec)
+     (if (listp spec)


@xiongtx That would return nil if spec is not a cons cell. I want to return spec.

raxod502 · 2018-03-22T01:01:59Z

@bbatsov All comments are addressed and tests are added. Thanks for your feedback. Also, thanks for such a nice test suite. It was actually fun to use :)

bbatsov · 2018-03-23T04:59:26Z

cider.el

    ("boot" (cider-boot-jack-in-dependencies
             global-opts
             params
             (cider-add-clojure-dependencies-maybe
              cider-jack-in-dependencies)
-             cider-jack-in-lein-plugins
-             cider-jack-in-nrepl-middlewares))
+             (cider-jack-in-normalized-lein-plugins)


I guess this is some odd legacy, but why are we passing lein plugins to boot? //cc @benedekfazekas

because they are implemented as such (both cider-nrepl and refactor-nrepl). boot either has machinery to handle them or just handles them as ordinary deps.

Yeah, I know why it was done. :-) I'm just saying it looks really strange to be passing something named lein-plugins to boot. For something like tools.deps and shadow-cljs the middleware packages would be regular deps.

right. unfortunately lein needs them separately. this is unfortunately the smallest denominator in terms of interface. have not checked the details of this PR yet but I am sure this can be improved.

bbatsov · 2018-03-23T05:02:19Z

While you're at this - the hardcoded middleware here should be replaced with the new function you introduced https://github.com/clojure-emacs/cider/blob/master/cider.el#L159

bbatsov · 2018-03-23T05:02:39Z

And this has to be rebased on top of master.

raxod502 · 2018-03-23T16:29:07Z

@bbatsov Regarding cider-clojure-cli-parameters, the most straight-forward solution seems to be to replace [\"cider.nrepl/cider-middleware\"] with %s and then have the variable run through format with (cider-jack-in-normalized-nrepl-middlewares) substituted in. Is that acceptable?

bbatsov · 2018-03-24T00:43:01Z

Yeah, that should work. Just test that it's working by jacking-in outside a project in with deps.edn project. Basically this just needs to build a vector of strings.

See clojure-emacs/clj-refactor.el#392 (comment)

raxod502 · 2018-03-24T17:08:28Z

@bbatsov Fixed, seems to be working.

bbatsov · 2018-03-27T12:46:21Z

👍

raxod502 mentioned this pull request Mar 14, 2018

[Fix #391] Add option for disabling warnings clojure-emacs/clj-refactor.el#392

Merged

6 tasks

bbatsov reviewed Mar 15, 2018

View reviewed changes

xiongtx reviewed Mar 19, 2018

View reviewed changes

raxod502 force-pushed the inject-predicates branch 3 times, most recently from 80bbf96 to fe98548 Compare March 22, 2018 00:58

bbatsov reviewed Mar 23, 2018

View reviewed changes

bbatsov force-pushed the master branch from 47f6fc8 to 93c2942 Compare March 24, 2018 02:03

Allow predicates in plugin and middleware lists

f9bd359

See clojure-emacs/clj-refactor.el#392 (comment)

raxod502 force-pushed the inject-predicates branch from fe98548 to f9bd359 Compare March 24, 2018 17:04

bbatsov merged commit ff43bb1 into clojure-emacs:master Mar 27, 2018

bbatsov mentioned this pull request Apr 11, 2018

Improve the way middleware is loaded with the clojure command and tools.deps #2226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow predicates in plugin and middleware lists #2238

Allow predicates in plugin and middleware lists #2238

raxod502 commented Mar 14, 2018 •

edited

Loading

bbatsov Mar 15, 2018

raxod502 Mar 15, 2018

bbatsov Mar 18, 2018

bbatsov Mar 24, 2018

bbatsov Mar 15, 2018

raxod502 Mar 15, 2018

bbatsov commented Mar 15, 2018

raxod502 commented Mar 15, 2018

raxod502 commented Mar 18, 2018

bbatsov commented Mar 19, 2018

xiongtx Mar 19, 2018

raxod502 Mar 22, 2018

xiongtx Mar 19, 2018

raxod502 Mar 19, 2018

raxod502 commented Mar 22, 2018

bbatsov Mar 23, 2018

benedekfazekas Mar 23, 2018

bbatsov Mar 23, 2018

benedekfazekas Mar 23, 2018

bbatsov commented Mar 23, 2018

bbatsov commented Mar 23, 2018

raxod502 commented Mar 23, 2018

bbatsov commented Mar 24, 2018

raxod502 commented Mar 24, 2018

bbatsov commented Mar 27, 2018

Allow predicates in plugin and middleware lists #2238

Allow predicates in plugin and middleware lists #2238

Conversation

raxod502 commented Mar 14, 2018 • edited Loading

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

bbatsov commented Mar 15, 2018

raxod502 commented Mar 15, 2018

raxod502 commented Mar 18, 2018

bbatsov commented Mar 19, 2018

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

raxod502 commented Mar 22, 2018

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 Mar 23, 2018

bbatsov commented Mar 23, 2018

raxod502 commented Mar 23, 2018

bbatsov commented Mar 24, 2018

raxod502 commented Mar 24, 2018

bbatsov commented Mar 27, 2018

raxod502 commented Mar 14, 2018 •

edited

Loading