Allow connecting to nbb via connect-clj (#3061) #3272

benjamin-asdf · 2022-12-01T08:50:00Z

This is for nbb, scittle, joyride.
Better support for "pain" repls, support "cljs" without setup.

we want to

connect with a plain client
make any assumptions explicit
connect cljs buffers with those plain repls
Check for cider middleware being present, before using it (already being done)
Check for cider-library-present-p, before relying on anything in the runtime
Make assumptions about the runtime explicit

My suggestion is =cider-connection-capabilities=.
Currently, there was an implicit assumption about the compilation error format.

=cider-connection-capabilities=
Is my suggestion for 2.

=cider-repls=
Now returns cljs, if the repl capabilities include 'cljs This way we can make a "plain" clj client, upgrade on connect with cljs capability and have it be connected in cljs buffers.

This is more a concession / workaround the current repl-type setup.

In the future we might get rid of repl-type?
The only reason we have it, and creaating issue 3., is because we want to be ergonomic about which buffer to use inside a given source buffer.
I found that I am able to juggle multiple clients by swapping to the buffer I want, thereby setting it active.

This can be a user command. Can also make a modeline element that swaps between clj/cljs like calva.

slack discussion:
https://clojurians.slack.com/archives/C04CAKAGADU

This pr also includes a few less consequential fixes that made nbb, scittle and joyride work.
(there was an error in the default repl init form "no such var, repl requires" )

The commits are consistent with our contribution guideglines
You've added tests (if possible) to cover your change(s)
All tests are passing (eldev test)
All code passes the linter (eldev lint) which is based on elisp-lint and includes
- byte-compilation,
  checkdoc,
  check-declare,
  packaging metadata, indentation, and trailing whitespace checks.
  
  I ran it but it is currently cluttered. I am still 90% certain I
  fixed the ones I would be introducing.
  
  If some warnings are ignored, please explain how to ignore.
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

benjamin-asdf · 2022-12-01T09:23:00Z

Don't merge I need to figure out a better way for checking "cljs present".
-> ok good now

This is for nbb, scittle, joyride. Better support for "pain" repls, support "cljs" without setup. we want to 1. connect with a plain client 2. make any assumptions explicit 3. connect cljs buffers with those plain repls 1. Check for cider middleware being present, before using it (already being done) 2. Check for cider-library-present-p, before relying on anything in the runtime 3. Make assumptions about the runtime explicit My suggestion is =cider-connection-capabilities=. Currently, there was an implicit assumption about the compilation error format. =cider-connection-capabilities= Is my suggestion for 2. =cider-repls= Now returns cljs, if the repl capabilities include 'cljs This way we can make a "plain" clj client, upgrade on connect with cljs capability and have it be connected in cljs buffers. This is more a concession / workaround the current repl-type setup. In the future we might get rid of repl-type? The only reason we have it, and creaating issue 3., is because we want to be ergonomic about which buffer to use inside a given source buffer. I found that I am able to juggle multiple clients by swapping to the buffer I want, thereby setting it active. This can be a user command. Can also make a modeline element that swaps between clj/cljs like calva. slack discussion: https://clojurians.slack.com/archives/C04CAKAGADU Format docstring Fix cljs check

bbatsov · 2022-12-01T11:30:42Z

Don't merge I need to figure out a better way for checking "cljs present".

You can also mark PRs as "Draft" to make this visually clear, btw.

bbatsov · 2022-12-01T11:32:25Z

cider-eval.el

-  (if (cider-nrepl-op-supported-p "stacktrace")
-      (cider-default-err-op-handler)
-    (cider-default-err-eval-handler)))
+  (cond ((cider-nrepl-op-supported-p "stacktrace") (cider-default-err-op-handler))


Seems to me this change won't result in anything different.

Ah, actually the similar names confused me. Now I see the last branch is different.

bbatsov · 2022-12-01T14:01:04Z

cider-connection.el

+(defvar-local cider-connection-capabilities '()
+  "A list of some of the capabilites of this connection buffer.
+Aka what assumptions we make about the runtime.
+This is more genaral than


bbatsov · 2022-12-01T14:01:45Z

cider-connection.el

+`cider-nrepl-op-supported-p' and `cider-library-present-p'.
+But does not need to replace them.")
+
+(defun cider-connection-has-capability-p (capability)


Shouldn't this operate only on the current buffer? Probably it'd be nice to be able to pass a connection to it explicitly.

bbatsov · 2022-12-01T14:02:25Z

cider-overlays.el

@@ -303,6 +303,7 @@ focused."
  (let* ((font-value (if cider-result-use-clojure-font-lock
                         (cider-font-lock-as-clojure value)
                       value))
+         (font-value (string-trim-right font-value))


I don't quite understand the purpose of this change.

nbb errors always had a trailing newline and the I think the visual of the overlays is better without the newline.

Got it. This name font-value confused me, as that's just value (which is potentially using font-locking). I was wondering why someone would be trimming the font-value. :D

bbatsov · 2022-12-01T14:03:16Z

cider.el

@@ -778,9 +778,13 @@ Generally you should not disable this unless you run into some faulty check."
  :safe #'booleanp
  :package-version '(cider . "0.17.0"))

+(defun cider-clojurescript-present-p ()
+  "Return non nil when ClojureScript is present."
+  (nrepl-dict-get (cider-sync-tooling-eval "cljs.core/demunge") "value"))


what's the advantage of this over the previous check?

#3255 Basically (require 'cljs.core) did not work for nbb.

bbatsov · 2022-12-02T20:16:38Z

cider-repl.el

@@ -153,7 +153,8 @@ you'd like to use the default Emacs behavior use
 (make-obsolete-variable 'cider-repl-print-level 'cider-print-options "0.21")

 (defvar cider-repl-require-repl-utils-code
-  '((clj . "(clojure.core/apply clojure.core/require clojure.main/repl-requires)")
+  '((clj . "(when-let [requires (resolve 'clojure.main/repl-requires)]


You'll have to explain me the need for this change.

When you currently connect to joyride it throws "cannot resolve symbol repl-requires". It was the same for an earlier version of nbb I basiclly want to make the clj connection as un-assuming as possible.

Might be good to mention such things in comments, so those things are clearer someone doesn't optimize this away. As @ikappaki mentioned in #3274 we don't really have integration tests, so it's not hard to imagine someone doing a change that would break this again for other runtimes.

We should also document somewhere that such REPL automatic util requires are limited to Clojure and the hosted ClojureScript.

bbatsov · 2022-12-02T20:19:42Z

cider-connection.el

+`cider-nrepl-op-supported-p' and `cider-library-present-p'.
+But does not need to replace them.")
+
+(defun cider-connection-has-capability-p (connection-buffer capability)


You can also swap the params if you want to make the connection optional (e.g. the current buffer or current-connection buffer by default).

bbatsov · 2022-12-02T20:22:32Z

Overall I like how this is shaping up. Apart from my small remarks I think it'd be nice to document somewhere that you can use cider-connect-clj for nbb & friends. Of course, after #3275 you'll be able to connect to native ClojureScript repls normally as well, but in this case I think more is better. :-) I guess we'll quickly make a lot of progress on that front.

benjamin-asdf · 2022-12-03T11:03:00Z

I added a doc in the docs scittle_and_friends.adoc

bbatsov · 2022-12-03T11:59:29Z

Thanks for tackling this! 🙇‍♂️

mk · 2022-12-03T13:00:02Z

@benjamin-asdf @bbatsov great to see this, thank you both!

jackrusher · 2022-12-03T14:10:00Z

@benjamin-asdf @bbatsov :D

benjamin-asdf force-pushed the connect-nbb branch from b37988b to fa2032a Compare December 1, 2022 09:09

benjamin-asdf force-pushed the connect-nbb branch from 7df9d1e to f75f017 Compare December 1, 2022 09:34

benjamin-asdf force-pushed the connect-nbb branch from f75f017 to ed8ee40 Compare December 1, 2022 09:49

bbatsov reviewed Dec 1, 2022

View reviewed changes

ikappaki mentioned this pull request Dec 2, 2022

Support nbb as native cljs repl #3275

Merged

6 tasks

Update connection-capability code

f0dc573

bbatsov reviewed Dec 2, 2022

View reviewed changes

benjamin-asdf added 2 commits December 3, 2022 10:32

Make buffer optional for cider-connection-has-capability-p

65b9ce1

Add a doc for using cider-connect-clj to connect to scittle,nbb..

d66c831

bbatsov merged commit 24b9891 into clojure-emacs:master Dec 3, 2022

ikappaki mentioned this pull request Dec 4, 2022

Error in process filter: ClojureScript is not available when jack'ing in to shadow #3277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow connecting to nbb via connect-clj (#3061) #3272

Allow connecting to nbb via connect-clj (#3061) #3272

benjamin-asdf commented Dec 1, 2022 •

edited

Loading

benjamin-asdf commented Dec 1, 2022 •

edited

Loading

bbatsov commented Dec 1, 2022

bbatsov Dec 1, 2022

bbatsov Dec 1, 2022

bbatsov Dec 1, 2022

benjamin-asdf Dec 2, 2022

bbatsov Dec 1, 2022

benjamin-asdf Dec 2, 2022

bbatsov Dec 1, 2022

benjamin-asdf Dec 2, 2022

bbatsov Dec 2, 2022

bbatsov Dec 1, 2022 •

edited

Loading

benjamin-asdf Dec 2, 2022

bbatsov Dec 2, 2022

benjamin-asdf Dec 3, 2022 •

edited

Loading

bbatsov Dec 3, 2022

bbatsov Dec 3, 2022

bbatsov Dec 2, 2022

benjamin-asdf Dec 3, 2022

bbatsov commented Dec 2, 2022

benjamin-asdf commented Dec 3, 2022

bbatsov commented Dec 3, 2022

mk commented Dec 3, 2022

jackrusher commented Dec 3, 2022

Allow connecting to nbb via connect-clj (#3061) #3272

Allow connecting to nbb via connect-clj (#3061) #3272

Conversation

benjamin-asdf commented Dec 1, 2022 • edited Loading

benjamin-asdf commented Dec 1, 2022 • edited Loading

bbatsov commented Dec 1, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamin-asdf Dec 3, 2022 • 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

bbatsov commented Dec 2, 2022

benjamin-asdf commented Dec 3, 2022

bbatsov commented Dec 3, 2022

mk commented Dec 3, 2022

jackrusher commented Dec 3, 2022

benjamin-asdf commented Dec 1, 2022 •

edited

Loading

benjamin-asdf commented Dec 1, 2022 •

edited

Loading

bbatsov Dec 1, 2022 •

edited

Loading

benjamin-asdf Dec 3, 2022 •

edited

Loading