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

[#955] Reuse existing repl buffer when cleint process has died #958

Merged
merged 2 commits into from
Mar 18, 2015

Conversation

vspinu
Copy link
Contributor

@vspinu vspinu commented Jan 23, 2015

This addresses only the user case when the port is preserved. When port is not preserved, new repl buffer is generated as before. This makes me wonder if port should be part of the buffer name in the first place. It makes repl buffer re-usage difficult.

@bbatsov
Copy link
Member

bbatsov commented Jan 24, 2015

When port is not preserved, new repl buffer is generated as before. This makes me wonder if port should be part of the buffer name in the first place. It makes repl buffer re-usage difficult.

I think we can check for dead REPL buffers and prompt the users for input if it's not clear what do to on cider-connect.

@bbatsov
Copy link
Member

bbatsov commented Jan 24, 2015

There's a typo in the commit message and there's no changelog entry.

@@ -156,7 +156,7 @@ as returned by `nrepl-connect'. ")
(concat nrepl-buffer-name-separator designation)
"")))

(defun nrepl-make-buffer-name (buffer-name-template &optional project-dir host port)
(defun nrepl-make-buffer-name (buffer-name-template &optional project-dir host port dup-ok)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few unit tests for this new behaviour would be useful.

@bbatsov
Copy link
Member

bbatsov commented Jan 31, 2015

@vspinu Ping :-)

@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

@vspinu Ping :-)

Sorry, I got involved with other stuff and forgot about this one.

I think we can check for dead REPL buffers and prompt the users for input if it's not clear what do to on cider-connect.

I think that if a dead-process-buffer is for the same project directory but the port differs we can safely reuse that buffer without even asking the user. On the other hand, if a dead-process-buffer is for a different project it rarely makes sense to reuse the buffer, so maybe it's not worth asking in this case either.

As to the unit tests. As the process handling system should be rewritten from scratch, such tests look to me like an unnecessary commitment at this stage.

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2015

I think that if a dead-process-buffer is for the same project directory but the port differs we can safely reuse that buffer without even asking the user. On the other hand, if a dead-process-buffer is for a different project it rarely makes sense to reuse the buffer, so maybe it's not worth asking in this case either.

I'm a bit confused - if we've used cider-connect we only have a host and a port; we don't actually have a project dir. So my idea was:

  • user runs cider-connect
  • we see there's some dead repl buffer that used to be connected to the same host, but on different port
  • we ask the user if he wants to reuse this dead repl buffer

As to the unit tests. As the process handling system should be rewritten from scratch, such tests look to me like an unnecessary commitment at this stage.

Fair enough.

@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

we see there's some dead repl buffer that used to be connected to the same host, but on different port

I thought you want to generalize this to cider-jack-in when the connection or server process dies.

How about querying for project directory on cider-connect?

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2015

I thought you want to generalize this to cider-jack-in when the connection or server process dies.

I'm also interested in being able to reuse this for the implementation of sensible restart semantics.

How about querying for project directory on cider-connect?

Doesn't feel right to me. Remote connections should be defined just in term of their endpoint IMO.

@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

Doesn't feel right to me. Remote connections should be defined just in term of their endpoint IMO.

Even if you work on remote you work on a project and having that information as part of the buffer name is useful IMO.

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2015

Even if you work on remote you work on a project and having that information as part of the buffer name is useful IMO.

Yeah, I can see some value in this. But even if we do it, the name should optional as some people might be connecting to the remote server just to do a bit of REPL debugging/inspection. In such cases they might not have a copy of the project's sources lying around.

@vspinu
Copy link
Contributor Author

vspinu commented Feb 1, 2015

In such cases they might not have a copy of the project's sources lying around.

I meant to simply request the project name from the remote and include it into repl buffer name. Having or not the source code lying around is irrelevant, unless I am missing something.

@bbatsov
Copy link
Member

bbatsov commented Feb 1, 2015

I meant to simply request the project name from the remote and include it into repl buffer name. Having or not the source code lying around is irrelevant, unless I am missing something.

Ah, I understand now. I assumed you wanted us to prompt the users for a "path to project" on connect and guessed the idea was that people are likely to have the project source around even if the server is remote. I wonder if the users will understand why they are supposed to set a project name, though.

@bbatsov
Copy link
Member

bbatsov commented Feb 13, 2015

So, what are we going to do about this?

@vspinu
Copy link
Contributor Author

vspinu commented Feb 14, 2015

So, what are we going to do about this?

I will amend it in light of what we have discussed. Unfortunately I cannot say when. Hopefully within a week.

vspinu added 2 commits March 18, 2015 12:51
…d died

Replace `nrepl-check-for-repl-buffer` with a smarter
`nrepl-find-reusable-repl-buffer`.
@vspinu
Copy link
Contributor Author

vspinu commented Mar 18, 2015

Ok, I have finally got down to it.

As discussed, if the zombi repl buffer matches exactly, the new connection is simply reusing the buffer. Otherwise the user is asked whether to reuse the existing zombi buffer and, if there are multiple such buffers, which of those to use.

Once you pull this one, I will make a new PR to changelog all my recent pulls.

bbatsov added a commit that referenced this pull request Mar 18, 2015
[#955] Reuse existing repl buffer when cleint process has died
@bbatsov bbatsov merged commit e290687 into clojure-emacs:master Mar 18, 2015
@vspinu vspinu deleted the 955 branch August 1, 2017 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants