Skip to content
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

(slurp *in*) can't be interrupted #2317

Closed
bbatsov opened this issue Jun 9, 2018 · 18 comments
Closed

(slurp *in*) can't be interrupted #2317

bbatsov opened this issue Jun 9, 2018 · 18 comments
Labels
bug good first issue A simple tasks suitable for first-time contributors

Comments

@bbatsov
Copy link
Member

bbatsov commented Jun 9, 2018

This is just creating a ticket for https://stackoverflow.com/questions/50493580/cider-for-emacs-stdin

It seems that slurping from the stdin right now can't be interrupted, which should certainly be possible.

@bbatsov bbatsov added bug low hanging fruit good first issue A simple tasks suitable for first-time contributors labels Jun 9, 2018
@TimoFreiberg
Copy link

I would like to contribute a bit to CIDER but I have no idea where to start with this issue. Could you/someone give me some pointers?

@dpsutton
Copy link
Contributor

@TimoFreiberg I made a few articles about how to start debugging and contributing: http://hackingcider.com/ (particularly setting up CIDER for development )

Also, feel free to chat in #cider on the Clojure slack

@bbatsov
Copy link
Member Author

bbatsov commented Jun 25, 2018

@TimoFreiberg
Copy link

thanks @dpsutton! M-x nrepl-toggle-message-loggin was the missing link for me :)

The problem seems to be:

Possible solutions seem to be:

  • Catching the signal 'quit from C-g and sending nil/an empty list to nrepl
  • Adding a C-c binding to the minibuffer which will also send nil/an empty list

I'll try that approach, feedback appreciated :)

@bbatsov
Copy link
Member Author

bbatsov commented Jun 25, 2018

Excellent research!

Yeah, I think we should do both.

cider-need-input always appends a newline to the input (which seems correct), therefore there is no way to send an empty stdin

Btw, I'm not quite sure about this. Why is it correct to always add a final newline?

@TimoFreiberg
Copy link

TimoFreiberg commented Jun 25, 2018

Btw, I'm not quite sure about this. Why is it correct to always add a final newline?

Otherwise it would be impossible to enter an empty line into stdin

@bbatsov
Copy link
Member Author

bbatsov commented Jun 25, 2018

You can just use \n. :-) But yeah - that's a good point. Otherwise we'll need some different keybinding (e.g. many chats have Shift+Enter for this). The approached proposed by you sounds better for sure.

@TimoFreiberg
Copy link

Happy to hear that :)
I'm gonna try to get it working, it might take a while since I'm new to emacs hacking ;)

@bbatsov
Copy link
Member Author

bbatsov commented Jun 25, 2018

You can see here how to tweak the minibuffer setup for stdin https://github.com/clojure-emacs/cider/blob/master/cider-common.el#L87

@bbatsov
Copy link
Member Author

bbatsov commented Jun 26, 2018

Btw, if you're the mood for more detective/bug fixing I can suggest 2 more small issues that are somewhat related:

@TimoFreiberg
Copy link

I'll take a look 👍

@TimoFreiberg
Copy link

TimoFreiberg commented Jun 26, 2018

Current status:
I bound C-c C-c to abort-recursive-edit.
C-c C-c and C-g successfully stops the stdin-loop:

https://gist.github.com/TimoFreiberg/34d7efa24f2966daff44601ce6c4753e#file-nrepl-exception

There's a problem though: calling (slurp *in*) again throws the following error:
https://gist.github.com/TimoFreiberg/34d7efa24f2966daff44601ce6c4753e#file-nrepl-messages-cider-issue-2317
probably because the stdin reader gets closed after reading an empty message.

@bbatsov
Copy link
Member Author

bbatsov commented Jun 27, 2018

Probably you're right. Not sure where would be best to handle this.

Btw, I'm curious if we can somehow add a way to conclude the input read by the minibuffer with EOF (e.g. with some keybinding) to prevent the need to send a second "termination" message.

@TimoFreiberg
Copy link

Oh, actually it's quite obvious. slurp closes *in* and nrepl just doesn't reopen it.

Yeah I've been thinking about that as well.
slurp reads until it encounters EOF, which nrepl sends when stdin is empty: https://github.com/clojure/tools.nrepl/blob/master/src/main/clojure/clojure/tools/nrepl/middleware/session.clj#L233
nrepl only does that when an empty message is sent, so I think an empty termination message is the only way unless nrepl changes

@bbatsov
Copy link
Member Author

bbatsov commented Jun 27, 2018

Well, improving things in nREPL is certainly doable now https://github.com/nrepl/nrepl/

CIDER will switch to using the new nREPL server pretty soon.

@bbatsov
Copy link
Member Author

bbatsov commented Jun 27, 2018

Anyways, for now we should probably go with a termination message, until the transition to the "new" nREPL is completed.

@TimoFreiberg
Copy link

TimoFreiberg commented Jun 27, 2018

I've got it now.

So we accept that users can close the nREPL stdin and maybe fix/improve that in the new nREPL project?

@bbatsov
Copy link
Member Author

bbatsov commented Jun 27, 2018

So we accept that users can close the nREPL stdin

Just for now.

maybe fix/improve that in the new nREPL project?

We should certainly fix this there, my point was mainly that some people will keep using the legacy project for a while, so ideally we have to be compatible with it as well.

CIDER will stop supporting the legacy nREPL as soon as I find some time to focus on this. :-)

TimoFreiberg pushed a commit to TimoFreiberg/cider that referenced this issue Jun 29, 2018
Add handler for the `quit` signal which sends an empty message to nREPL
Add C-c C-c binding for `abort-recursive-edit` (which sends a `quit` signal)
TimoFreiberg pushed a commit to TimoFreiberg/cider that referenced this issue Jun 29, 2018
Add handler for the `quit` signal which sends an empty message to nREPL
Add C-c C-c binding for `abort-recursive-edit` (which sends a `quit` signal)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue A simple tasks suitable for first-time contributors
Projects
None yet
Development

No branches or pull requests

3 participants