-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
Extend the "describe" op with some extra runtime information #181
Comments
In the case of babashka, should the runtime be
|
I like this idea, except I really don't think "protocol-version" should be included at this point.
Conflating the protocol with the reference implementation is a very bad idea. If we come up with an independent document describing the protocol on its own and version that separately, then it could make sense to include this in the description, but I'm not aware of any such thing at this time.
|
I finally started working on this today, so I'll have some update soon. Now I'm leaning towards adding a @borkdude In your case runtime should be "babaska", "name" whatever you want (e.g. {:runtime "babashka"
:name "nrepl-babashka"
:versions { :babashka "..."
:java "..."
:nrepl-babashka "..."}} Let me know how this sounds to you and whether you have some preferences about the names of the extra keys. |
Btw, I guess for babashka the runtime can easily be clojure as well, given how highly compatible they are. That's not a big deal in the end of the day I think. |
Having a separate version / java from version / babashka doesn't make sense, since you cannot have different Java/GraalVM versions with the same babashka version, i.e it's a derived property. From 0.1.0 on babashka is compiled with GraalVM java11 (right now GraalVM version 20.1.0). I think the field "name" could have a more descriptive name. What is this going to be used for? In the case of babashka there is only one possibility: [babashka.nrepl] To summarize, I think for babashka this makes sense:
and that's about it? |
Got it. Well, one less version then.
For most runtimes there's just one server currently, so potentially we can skip this. There's no guarantee there won't be more servers down the road, but there's also no real need for premature optimization at this point.
Fine by me. |
Btw, doesn't your nREPL server have different versioning from |
@bbatsov True that. We could add |
We have a deal! 🤝 |
@bbatsov There are other projects that use babashka.nrepl as their nREPL server. Their runtime, like babashka, is based on the sci interpreter. I made a proposal here for these projects: Is this information going to be used by CIDER? Would it be useful to have one common key so CIDER doesn't have to add cases for each new sci-based project? It's not always clear what a sci-based project will or won't support though, since sci is just an interpreter. What it can provide depends on how people use the interpreter. So it's likely that an nREPL client will still have to try things with |
Yeah, it will be. The plan is to have CIDER support better non-Clojure nREPL servers See clojure-emacs/cider#2848 for more details. As an immediate step for babashka I wanted to make some changes that would eliminate the warnings they are getting from CIDER today. Unlike a language like Fennel it doesn't really require much internal changes, as most of the Clojure code that CIDER runs will work just fine.
Well, if the runtime is not Clojure-like we can't even try |
@bbatsov If that's the goal, maybe for runtime it's better to use some namespaced thing, so the project can describe it's some flavor of Clojure?
So then CIDER knows by the namespace of the key that it can use Or a separate |
I've been following this conversation with some trepidation, as it reminds me of how the web has struggled to improve because of everyone doing user agent checks. Instead of checking for implementation/version it would be more robust and forward thinking to announce and check for specific capabilities... |
@plexus Good point. That's exactly why I have avoided communicating a pod version in babashka.pods but so far have only communicated capabilities using the Stating a Btw, this was weird: |
Yeah, maybe we should have a separate key about the language, as with ClojureScript you can have both a Java runtime and a self-hosted JavaScript runtime and you need to be able to differentiate between the two. On the other hand from what I gather some servers like Ogion can handled two languages (Lua and Fennel), even if the server is implemented in Lua and obviously Lua is the actual runtime (as it's Java for regular Clojure). With hosted languages we have two layers of runtime which makes things a bit more complicated. |
The problem is that many clients patch some missing capabilities by evaluating code and they need to know what exactly to evaluate. If we remove the evaluations from the equation we don't really care about the runtime. If we need to auto-detect a connection type some extra info would be useful as well. But now I'm wondering if I'm not approaching this from the wrong angle. :-)
Yeah, this might work as well. |
Maybe good to take a step back. Start with a clear problem statement. Then explore alternative solutions. Then choose one minimal solution that could be extended later if necessary. To restate the problem: CIDER (or other editor) is missing information about the nREPL server. It could try |
Probably there are other options, but those two are the obvious ones. |
Can you elaborate on 2? Not sure I understand. |
E.g. you want to provide code completion by just evaluating code in some code completion library (as opposed to a dedicated op) - that's how Leiningen currently provides code completion in its REPL. That's a common approach many people take, as it's the simplest thing to do in many cases and it's the main issue when it comes to clients portability. |
Btw, there's one more nuance we have to keep in mind - how do you probe for capabilities that are not actually ops (e.g. the |
Seems the conversation here got stalled, so I won't change anything for time being. @borkdude can just add this to babashka.nrepl:
So I can teach CIDER about it. I played a bit with babashka and CIDER today and all the nil versions and warnings on startup are something I'd like to fix. |
Will do! |
For the record I'm 100% in favor of this; at the very least implementing functionality using |
Is your feature request related to a problem? Please describe.
Today it's hard to tell what's the runtime of the server you're connecting to (e.g. Clojure, ClojureScript, ClojureCLR, babashka, or some non-Clojure language like Racket, fennel, etc.).
One practical problem is that client often implement some functionality in terms of evaluating runtime-specific code, which often means that one client can't be used with different nREPL servers simply because it makes some assumptions that are not universally true.
If clients could easily infer that runtime of the server they can support easily different runtimes.
Describe the solution you'd like
I think we should agree that every nREPL server should return some extra metadata in the describe op:
nrepl-version
.nrepl-version
I guess)E.g. you can have something like:
Describe alternatives you've considered
I didn't manage to think of a good alternative.
@nrepl/nrepl @technomancy @borkdude setzer22 Let me know how this sounds to you.
The text was updated successfully, but these errors were encountered: