[Fix #391] Add option for disabling warnings #392

raxod502 · 2017-10-15T18:32:46Z

Add cljr-suppress-outside-project-warning defcustom. Set cider-lein-* variables using advice-driven let-binding instead of global modification, and only inject nrepl-middleware when inside project context. After REPL has started, if outside project context, display more specific warning (unless the new defcustom is set to non-nil).

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
- No, as existing warnings were not tested
The new code is not generating byte compile warnings (run cask exec emacs -batch -Q -L . -eval "(progn (setq byte-compile-error-on-warn t) (batch-byte-compile))" clj-refactor.el)
All tests are passing (run ./run-tests.sh)
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the readme (if adding/changing user-visible functionality)
- No, as existing variable cljr-suppress-middleware-warnings was not documented there

raxod502 · 2017-10-15T18:36:59Z

I don't know why the build is failing, but it does not seem relevant to my change.

benedekfazekas · 2017-10-15T19:57:35Z

clj-refactor.el

+             (cons "refactor-nrepl.middleware/wrap-refactor"
+                   cider-jack-in-nrepl-middlewares)))
+        (apply cider-jack-in args))
+    (apply cider-jack-in args)))


why do you prefer cons over add-to-list?

Because add-to-list would modify the list variable globally, unless I let-bound the variable to itself (which would be more complicated than just using cons).

I see no harm in modifying these variables globally. Am I missing any edge cases here?

If the variables were modified globally, then that would cause refactor-nrepl to be injected always, as opposed to just when you are inside a project. Avoiding that behavior is the whole point of this patch.

benedekfazekas · 2017-10-15T19:59:06Z

clj-refactor.el

-    (add-to-list 'cider-jack-in-lein-plugins `("refactor-nrepl" ,(cljr--version t)))
-    (add-to-list 'cider-jack-in-nrepl-middlewares "refactor-nrepl.middleware/wrap-refactor")))
+  (if (and (not (string-empty-p (cljr--project-dir)))
+           cljr-inject-dependencies-at-jack-in)


keeping the boundp checks on the cider vars make sense to make sure that the cider version installed is compatible with this feature... any particular reason you got rid of them?

Oops, I was originally thinking we didn't need them since I switched from add-to-list to let + cons, but we still do since I use the original value. Fixing that now.

benedekfazekas · 2017-10-15T20:02:40Z

apart from the comments i am 👍 to merge this. build failure is weird indeed. perhaps something with cask?!

expez · 2017-10-16T08:07:06Z

clj-refactor.el


 ;;;###autoload
 (eval-after-load 'cider
-  '(cljr--inject-jack-in-dependencies))
+  '(advice-add #'cider-jack-in :around #'cljr--inject-jack-in-dependencies))


@bbatsov This seems like a no-no to me. What are you thoughts on using advice here?

For context, @raxod502 is submitting a patch so a variable will prevent the middleware dep from even being injected when we're outside a project. The reason being that the middleware will emit warnings that he doesn't care about.

The reason I used advice here was that advice was already being used elsewhere; see:

clj-refactor.el/clj-refactor.el

Line 1638 in f5295df

(advice-add 'paredit-convolute-sexp :after #'clojure--replace-let-bindings-and-indent)))

Note also that mutating a global variable in an autoload is not considered good practice either, so I don't think either the previous code or the new code is ideal.

If you really don't want to use advice, I'll be happy to submit a pull request upstream for some additional customizable variables to be added so that doing this doesn't require advice.

expez · 2017-10-16T08:08:23Z

clj-refactor.el

@@ -155,6 +155,14 @@ as can be."
  :group 'cljr
  :type 'boolean)

+(defcustom cljr-suppress-outside-project-warning nil


I think cljr-suppress-no-project-warning might be a bit more self-explanatory.

Good idea. Changed.

expez · 2017-10-16T08:10:23Z

clj-refactor.el

+(defcustom cljr-suppress-outside-project-warning nil
+  "If t, no warning is printed when starting a REPL outside a project.
+By default, a warning is printing in this case since clj-refactor
+will not work as expected in such REPLs and therefore the


Cut everything after "and therefore", as that last part only confuses things.

Of course, we're still using my fork until [1] and [2] are merged. [1]: clojure-emacs/clj-refactor.el#392 [2]: clojure-emacs/clj-refactor.el#385

raxod502 · 2017-12-18T04:39:37Z

Any update here?

expez · 2017-12-18T11:23:52Z

Any update here?

We had another user requesting this recently :)

I'd like to take you up on the offer to submit a PR to CIDER to avoid the use of advice.

You're right that we have some advice for paredit, but if that advice goes stale, and breaks, everything else works fine, whereas if the advice for cider breaks down, due to changes upstream, we're likely to break both CIDER and clj-refactor for everyone.

The changelog entry is also added in the wrong place (in the release notes for an already released version). That's on us, I guess, for not pushing a an 'Upcoming' section. Sorry about that.

bbatsov · 2017-12-18T12:59:31Z

Indeed!

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

raxod502 · 2018-03-14T22:29:05Z

I've created a pull request at clojure-emacs/cider#2238. Apologies for the delay in doing so.

raxod502 · 2018-03-14T22:29:59Z

I've also updated this pull request to use the :predicate feature introduced therein, so advice is no longer required.

expez · 2018-03-16T07:58:01Z

clj-refactor.el

@@ -155,6 +155,13 @@ as can be."
  :group 'cljr
  :type 'boolean)

+(defcustom cljr-suppress-no-project-warning nil
+  "If t, no warning is printed when starting a REPL outside a project.
+By default, a warning is printing in this case since clj-refactor


printing -> printed

expez · 2018-03-16T08:05:21Z

clj-refactor.el

@@ -786,6 +793,10 @@ A new record is created to define this constructor."
     car)
   ""))

+(defun cljr--inside-project-p ()


Think this should be called cljr--inject-middleware-p? to better reflect its usage.

Also, shouldn't it have an argument list like (&rest _) since it's actually expecting 3 arguments, that are ignored?

Edit: I see it's used in two places, so I'd suggest leaving this and creating another function, cljr--inject-middleware-p that just has the right argument list and calls out to this one.

raxod502 · 2018-03-16T16:39:39Z

Thanks, fixed.

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

* Add `cljr-suppress-outside-project-warning' defcustom. * Use new `:predicate' support in `cider-jack-in-lein-plugins' and `cider-jack-in-nrepl-middlewares' to only inject when inside project context. (Bump CIDER dependency to 0.17.0.) * After REPL has started, if outside project context, display better warning than before (unless the new defcustom is set to non-nil).

raxod502 · 2018-03-22T02:49:54Z

@expez Now that the pull request for CIDER is functional and tested, I fixed a few things here and tested. Appears to be working.

expez · 2018-03-22T08:16:01Z

Great! 👍

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

raxod502 · 2018-03-28T16:51:34Z

@expez Well, the pull request has been merged upstream. Now we can look at this one :)

expez · 2018-03-30T09:13:44Z

Great work @raxod502!

Thanks for seeing this through all the way! 👍

raxod502 force-pushed the feat/suppress-warnings branch from 8fa901c to 506add8 Compare October 15, 2017 18:32

raxod502 mentioned this pull request Oct 15, 2017

Can clj-refactor.el degrade cleanly outside a project context? #391

Closed

benedekfazekas reviewed Oct 15, 2017

View reviewed changes

raxod502 force-pushed the feat/suppress-warnings branch from 506add8 to 9ee0521 Compare October 15, 2017 23:53

expez reviewed Oct 16, 2017

View reviewed changes

raxod502 force-pushed the feat/suppress-warnings branch from 9ee0521 to dbf7f0c Compare October 16, 2017 15:43

raxod502 added a commit to radian-software/radian that referenced this pull request Oct 22, 2017

Remove clj-refactor hacks

240a316

Of course, we're still using my fork until [1] and [2] are merged. [1]: clojure-emacs/clj-refactor.el#392 [2]: clojure-emacs/clj-refactor.el#385

raxod502 added a commit to raxod502/cider that referenced this pull request Mar 14, 2018

Allow predicates in plugin and middleware lists

5934aba

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

raxod502 mentioned this pull request Mar 14, 2018

Allow predicates in plugin and middleware lists clojure-emacs/cider#2238

Merged

6 tasks

raxod502 force-pushed the feat/suppress-warnings branch from dbf7f0c to b88dd5b Compare March 14, 2018 22:29

expez requested changes Mar 16, 2018

View reviewed changes

raxod502 force-pushed the feat/suppress-warnings branch from b88dd5b to 51dda2f Compare March 16, 2018 16:39

expez approved these changes Mar 19, 2018

View reviewed changes

raxod502 added a commit to raxod502/cider that referenced this pull request Mar 22, 2018

Allow predicates in plugin and middleware lists

427a40f

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

raxod502 added a commit to raxod502/cider that referenced this pull request Mar 22, 2018

Allow predicates in plugin and middleware lists

80bbf96

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

raxod502 added a commit to raxod502/cider that referenced this pull request Mar 22, 2018

Allow predicates in plugin and middleware lists

fe98548

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

raxod502 force-pushed the feat/suppress-warnings branch from 51dda2f to a15fef7 Compare March 22, 2018 01:04

raxod502 added a commit to raxod502/cider that referenced this pull request Mar 24, 2018

Allow predicates in plugin and middleware lists

f9bd359

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

bbatsov pushed a commit to clojure-emacs/cider that referenced this pull request Mar 27, 2018

Allow predicates in plugin and middleware lists

ff43bb1

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

expez merged commit b230432 into clojure-emacs:master Mar 30, 2018

raxod502 deleted the feat/suppress-warnings branch March 30, 2018 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #391] Add option for disabling warnings #392

[Fix #391] Add option for disabling warnings #392

raxod502 commented Oct 15, 2017 •

edited

Loading

raxod502 commented Oct 15, 2017

benedekfazekas Oct 15, 2017

raxod502 Oct 15, 2017

benedekfazekas Oct 16, 2017

raxod502 Oct 16, 2017

benedekfazekas Oct 15, 2017

raxod502 Oct 15, 2017

benedekfazekas commented Oct 15, 2017

expez Oct 16, 2017 •

edited

Loading

raxod502 Oct 16, 2017 •

edited

Loading

raxod502 Oct 16, 2017

expez Oct 16, 2017

raxod502 Oct 16, 2017

expez Oct 16, 2017

raxod502 Oct 16, 2017

raxod502 commented Dec 18, 2017

expez commented Dec 18, 2017

bbatsov commented Dec 18, 2017

raxod502 commented Mar 14, 2018

raxod502 commented Mar 14, 2018

expez Mar 16, 2018

expez Mar 16, 2018

raxod502 commented Mar 16, 2018

raxod502 commented Mar 22, 2018

expez commented Mar 22, 2018

raxod502 commented Mar 28, 2018

expez commented Mar 30, 2018

[Fix #391] Add option for disabling warnings #392

[Fix #391] Add option for disabling warnings #392

Conversation

raxod502 commented Oct 15, 2017 • edited Loading

raxod502 commented Oct 15, 2017

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 Oct 15, 2017

expez Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

raxod502 Oct 16, 2017 • 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

raxod502 commented Dec 18, 2017

expez commented Dec 18, 2017

bbatsov commented Dec 18, 2017

raxod502 commented Mar 14, 2018

raxod502 commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raxod502 commented Mar 16, 2018

raxod502 commented Mar 22, 2018

expez commented Mar 22, 2018

raxod502 commented Mar 28, 2018

expez commented Mar 30, 2018

raxod502 commented Oct 15, 2017 •

edited

Loading

expez Oct 16, 2017 •

edited

Loading

raxod502 Oct 16, 2017 •

edited

Loading