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

Rewrite nrepl callback handling #1099

Open
vspinu opened this Issue May 12, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@vspinu
Contributor

vspinu commented May 12, 2015

This is a continuation of the conversation at #1098

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu May 12, 2015

Contributor

One idea was to drop buffer argument in handlers and provide a macro that would create a handler with current buffer and current connection stored in its closure.

The lexical approach is awkward with respect to the composition of the handlers that @cichli proposed. So, I guess, this leaves us with the dynamic approach only.

Contributor

vspinu commented May 12, 2015

One idea was to drop buffer argument in handlers and provide a macro that would create a handler with current buffer and current connection stored in its closure.

The lexical approach is awkward with respect to the composition of the handlers that @cichli proposed. So, I guess, this leaves us with the dynamic approach only.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov May 12, 2015

Member

Btw, I understand why we need the connection, but why do we need the current/another buffer? Not everything acts on it, so maybe the handlers should just obtain the buffers they need. Passing some buffer to all handlers always seems wrong to me.

Member

bbatsov commented May 12, 2015

Btw, I understand why we need the connection, but why do we need the current/another buffer? Not everything acts on it, so maybe the handlers should just obtain the buffers they need. Passing some buffer to all handlers always seems wrong to me.

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu May 12, 2015

Contributor

but why do we need the current/another buffer? Not everything acts on it, so maybe the handlers should just obtain the buffers they need.

I see at least two reasons for that.

Most importantly, this is useful for "global" handlers that CIDER or the USER can insert into the chain of handlers. Most common example is caching. As with (setq cider-buffer-ns ns) in current nrepl-make-response-handler which is always called when value is present.

Secondly, if you don't bind the buffer automatically then the handler has to do that itself (all eval handlers for example). This means that you cannot simply define a callback and then store only its name in nrepl-pending-requests. You will still need to define a lambda-generator, call it in the code and then store the whole calllback lambda in the request hash table. I am pretty sure if we store the current buffer we will reduce the use of closures as callbacks almost entirely.

I think the general idea is to store a complete context of the invocation and it's rather independent whether the handler wants to use it or not. For completeness I would also include the current point in this list (a marker actually). Examples are error handlers that must invoke a jump, and would like to push mark or xref-mark before hand. It might be useful for asyncronuous completion as well. Imagine asyncronous eldoc which would inhibit message display if the point moved in the meanwhile.

A more general idea would be to have nrepl-defhandler macro which would allow to store arbitrary context. You would call it like (nrepl-defhandler my-handler ((buf (current-bufer)) (mode major-mode)) &rest body) but that basically means re-implementing closures which you can store nicely in the nrepl-pending-requests structure. Not sure it's worth the bang.

Contributor

vspinu commented May 12, 2015

but why do we need the current/another buffer? Not everything acts on it, so maybe the handlers should just obtain the buffers they need.

I see at least two reasons for that.

Most importantly, this is useful for "global" handlers that CIDER or the USER can insert into the chain of handlers. Most common example is caching. As with (setq cider-buffer-ns ns) in current nrepl-make-response-handler which is always called when value is present.

Secondly, if you don't bind the buffer automatically then the handler has to do that itself (all eval handlers for example). This means that you cannot simply define a callback and then store only its name in nrepl-pending-requests. You will still need to define a lambda-generator, call it in the code and then store the whole calllback lambda in the request hash table. I am pretty sure if we store the current buffer we will reduce the use of closures as callbacks almost entirely.

I think the general idea is to store a complete context of the invocation and it's rather independent whether the handler wants to use it or not. For completeness I would also include the current point in this list (a marker actually). Examples are error handlers that must invoke a jump, and would like to push mark or xref-mark before hand. It might be useful for asyncronuous completion as well. Imagine asyncronous eldoc which would inhibit message display if the point moved in the meanwhile.

A more general idea would be to have nrepl-defhandler macro which would allow to store arbitrary context. You would call it like (nrepl-defhandler my-handler ((buf (current-bufer)) (mode major-mode)) &rest body) but that basically means re-implementing closures which you can store nicely in the nrepl-pending-requests structure. Not sure it's worth the bang.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov May 12, 2015

Member

I know why it's done as it is. My problem is that there are plenty of handlers that just ignore the "attached" buffer and currently this isn't even an optional param.

I think the general idea is to store a complete context of the invocation and it's rather independent whether the handler wants to use it or not. For completeness I would also include the current point in this list (a marker actually). Examples are error handlers that must invoke a jump, and would like to push mark or xref-mark before hand. It might be useful for asyncronuous completion as well. Imagine asyncronous eldoc which would inhibit message display if the point moved in the meanwhile.

A more general idea would be to have nrepl-defhandler macro which would allow to store arbitrary context. You would call it like (nrepl-defhandler my-handler ((buf (current-bufer)) (mode major-mode)) &rest body) but that basically means re-implementing closures which you can store nicely in the nrepl-pending-requests structure. Not sure it's worth the bang.

Yeah, those are nice points. I have to spend more time thinking about this, but in the mean time - more ideas are welcome!

Member

bbatsov commented May 12, 2015

I know why it's done as it is. My problem is that there are plenty of handlers that just ignore the "attached" buffer and currently this isn't even an optional param.

I think the general idea is to store a complete context of the invocation and it's rather independent whether the handler wants to use it or not. For completeness I would also include the current point in this list (a marker actually). Examples are error handlers that must invoke a jump, and would like to push mark or xref-mark before hand. It might be useful for asyncronuous completion as well. Imagine asyncronous eldoc which would inhibit message display if the point moved in the meanwhile.

A more general idea would be to have nrepl-defhandler macro which would allow to store arbitrary context. You would call it like (nrepl-defhandler my-handler ((buf (current-bufer)) (mode major-mode)) &rest body) but that basically means re-implementing closures which you can store nicely in the nrepl-pending-requests structure. Not sure it's worth the bang.

Yeah, those are nice points. I have to spend more time thinking about this, but in the mean time - more ideas are welcome!

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu May 12, 2015

Contributor

A caller making an nREPL request will then simply construct their response handler using function composition:

(->> (cider-repl--out-response-handler buffer)
(cider-repl--value-response-handler buffer)
(cider--ns-response-handler)
...)

This is basically building middlware stacks each time you make a request. Seems rather cumbersome to me with no real gain. First, you have to read this backwards because the last one is executed first. From technical prospective, you store a huge lambda in the request cache. From a programer prospective it's rather inconvenient because of two competing concepts - handler and middleware that dynamically generates the handler (BTW, -handler postfix is inappropriate here. CIDER's usage of the term handler also seems wrong.).

How about a simpler linear dispatch a la capf? You have a global nrepl-handlers list which contains functions that take one argument - the response and returns a (possibly altered) response object or nil. The response object is passed in turn from one handler to the next handler. Each of these functions is called in turn till one of them returns nil. If all functions returned non-nil, the callback origiinaly reggisterd by the caller is finally called.

You can also compose handlers directly:


(nrepl-send-request request-obj 
                    (-compose 'handler1
                              'handler2
                              ....))

You can still use generators to dynamically produce closures that capture locals, but if we already capture the context (connection, buffer and point) this will be rarely needed.

Contributor

vspinu commented May 12, 2015

A caller making an nREPL request will then simply construct their response handler using function composition:

(->> (cider-repl--out-response-handler buffer)
(cider-repl--value-response-handler buffer)
(cider--ns-response-handler)
...)

This is basically building middlware stacks each time you make a request. Seems rather cumbersome to me with no real gain. First, you have to read this backwards because the last one is executed first. From technical prospective, you store a huge lambda in the request cache. From a programer prospective it's rather inconvenient because of two competing concepts - handler and middleware that dynamically generates the handler (BTW, -handler postfix is inappropriate here. CIDER's usage of the term handler also seems wrong.).

How about a simpler linear dispatch a la capf? You have a global nrepl-handlers list which contains functions that take one argument - the response and returns a (possibly altered) response object or nil. The response object is passed in turn from one handler to the next handler. Each of these functions is called in turn till one of them returns nil. If all functions returned non-nil, the callback origiinaly reggisterd by the caller is finally called.

You can also compose handlers directly:


(nrepl-send-request request-obj 
                    (-compose 'handler1
                              'handler2
                              ....))

You can still use generators to dynamically produce closures that capture locals, but if we already capture the context (connection, buffer and point) this will be rarely needed.

@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu May 13, 2015

Contributor

(BTW, -handler postfix is inappropriate here. CIDER's usage of the term handler also seems wrong.).

Never mind. It's a handler constructor, not a handler itself, so it's fine to call it something-handler. I just need more sleep.

Contributor

vspinu commented May 13, 2015

(BTW, -handler postfix is inappropriate here. CIDER's usage of the term handler also seems wrong.).

Never mind. It's a handler constructor, not a handler itself, so it's fine to call it something-handler. I just need more sleep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment