Enhance scittle nrepl, where to add stuff?

[benjamin-asdf]

Currently we have this 2 layer solution for the sci nrepl that talks via websocked to e.g. scittle nrepl. I would like to implement eldoc for scittle so I would have to add it both here and there; Sounds like trouble. In the case of eldoc we might be able to implement in terms of sci.nrepl. Because it only needs to eval

(meta
 (resolve sym))

I guess the whole point of sci.nrepl is to try to not have to reimplement the same stuff for each project.

[borkdude]

It would be nice if we can concentrate the logic here indeed.

[benjamin-asdf]

Describe Operation Implementation Challenge

The browser server handles describe, but which ops are actually implemented is defined on the handler side. Suggested naming:

So client <- browser-server -> nrepl-handler-server (scittle.nrepl).

If I want to add eldoc, I want to advertise the supported op (ops in describe).

1. Make browser-server always say "eldoc" is implemented .
2. Make browser-server flow describe through to handler-server, so it handles it by itself. (This could be done by a sci.nrepl lib function, that server-handler can use).

1. downsides:

• Would not be correct anymore, if we plug in a another server-handler (which handles a different set of ops)

2. downsides:

• More logic on the handler side but we want as much as possible in sci.nrepl
• Braking because existing implementations need to handle describe

[borkdude]

I'll get back to this hopefully soon.

[borkdude]

@benjamin-asdf

1. I think we need to move the describe response to the implementation (scittle) rather than this dependency since it will depend on the implementation what it supports.

2. As for eldoc: we can put the common eldoc handler in sci.nrepl and then you can hook it up in the implementation, similar to how completions work.

nbb has a slightly more sophisticated eldoc handler than what you described:

https://github.com/babashka/nbb/blob/ec00dfd7e8de51a85db4318f52decc103c98c2ee/src/nbb/impl/nrepl_server.cljs#L194-L244

[benjamin-asdf]

Yes this makes the most sense :+1:

I guess we already know that nobody else is using sci.nrepl except the scittle repl? Because it would be a braking change if they update sci.nrepl.. They then need to implement describe else tools would break.

I plan to tackle moving describe and implement an eldoc for scittle repl in the comming days.

[borkdude]

sci.nrepl isn't really used beyond scittle. There is an open PR in Clerk that adds the same nREPL support as scittle to it, but I can easily change that.

[benjamin-asdf]

https://github.com/babashka/sci.nrepl/blob/b75700da7797f9fc2b4ef1bef9df7230f9fd1e8c/src/sci/nrepl/browser_server.clj#L120

How to handle the ops list? Currently, if I add additional ops handled by the implementing nrepl-server,

1) we also need some scheme to know about the ops.

2) Or we flow all ops through to the nrepl-server and expect it to complain about unkown ops itself?

maybe 2 is the simplest and easiest. I would then not complain about unkown op in browser_server and assume the nrepl-server is handleling ops / unkown ops.

[borkdude]

I don't fully understand the consequences of either, so just go with the easiest option and we will change if necessary

[benjamin-asdf]

eldoc in scittle! haha

not all core functions have meta data. str for example does not.

[borkdude]

not all core functions have meta data. str for example does not.

This is because scittle is compiled with SCI_ELIDE_VARS which is a setting which leaves out docstrings and arglists for core vars to save space.