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

Improve prompts for reusing dead REPLs #3354

Merged
merged 8 commits into from
Jun 16, 2023
Merged

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Jun 15, 2023

Fixes the regression mentioned in #3353, which caused dead clj REPL buffers to not prompt for reuse.
Also changes the wording of the prompt as suggested in #3076, and implements a defcustom, allowing the user to always / never reuse dead repls. Defaults to the current behaviour of always prompting.
I also added my preferred auto behaviour which skips the prompt when there's a single unambiguous y/n choice.

Fixes #3353
Closes #3076


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

Another suggestion - should answering "no" to the prompt kill the dead REPL buffer instead of keeping it around?
I imagine that someone who chooses to not reuse the REPL wouldn't want to be prompted about the same buffer again the next time they open a connection. See also #3254 (comment)

Maybe another prompt "Kill dead REPL buffer(s)? (y or n)" after they reply no to the first prompt, to make it more explicit?
We could also use read-char-choice to present 3 choices, although that feels a bit much:
[y]es - reuse / [n]o - ignore / [k]ill
[y]es - reuse / [n]o - kill / [i]gnore

@bbatsov
Copy link
Member

bbatsov commented Jun 16, 2023

I imagine that someone who chooses to not reuse the REPL wouldn't want to be prompted about the same buffer again the next time they open a connection. See also #3254 (comment)

That'd be fine by me. We didn't kill it initially, as the assumption was that there might be some useful info in the dead buffer that people might want to keep around. Probably that's rarely the case.

@bbatsov bbatsov merged commit 049eb30 into clojure-emacs:master Jun 16, 2023
@bbatsov
Copy link
Member

bbatsov commented Jun 16, 2023

I'll merge your PR in its current state, as it fixes the regression that I had introduced, and we can keep doing/discussing improvements to the reuse functionality separately. Thanks for tackling this!

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

Thanks! I just spotted a small inconsistency / bug with the new defcustom docstring and cond logic - when the value is not one of the standard choices it should fall through to the any case, instead of prompting.

I was also going to submit another PR for the cider-auto-mode fix, if it's alright you can let me know re. the killing / cleanup functionality and I'll include them along with the PR.

@bbatsov
Copy link
Member

bbatsov commented Jun 16, 2023

@yuhan0 Yeah, you can include the killing logic in your follow-up PR.

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

Would you prefer the double prompt (reuse? y/n) -n-> (kill? y/n) or a single 3-choice prompt?

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

I've also found that in very rare cases the call to cider--gather-connect-params can throw an error and cause the entire connection process to be aborted, so it's probably a good idea to defensively wrap the entire thing in a ignore-errors form.

(One of my ad-hoc convenience commands used (cider-font-lock-as major-mode ...), which once generated a background cider-repl-mode temp buffer without the expected local vars)

@yuhan0 yuhan0 mentioned this pull request Jun 16, 2023
6 tasks
:group 'cider
:safe #'symbolp
:package-version '(cider . "1.5"))

(defcustom cider-reuse-dead-repls 'prompt
Copy link
Member

Choose a reason for hiding this comment

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

Could a nil default introduce less friction?

I think I've seen 2-3 times user feedback like this:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind it either way, but it seems like @bbatsov prefered to keep the prompt behaviour as a default, from the discussion in #3076.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it's so good to have that discussion in the archives :)

Alright then. Perhaps we can change...?

-A dead REPL %s exists.  Reuse buffer?
+A dead REPL %s exists.  Reuse buffer? (see also: `cider-reuse-dead-repls' defcustom)

Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit too much to mention the defcustom, as we never do this normally. And it this change probably doesn't affect people much anyways.

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.

Regression - Cider no longer asks to reuse clj REPL buffers Consider changing dead repl prompt text
3 participants