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

Tidying up switch-to-relevant-buffer. #451

Merged
merged 1 commit into from
Jan 14, 2014
Merged

Tidying up switch-to-relevant-buffer. #451

merged 1 commit into from
Jan 14, 2014

Conversation

jonpither
Copy link
Contributor

Hi,

What's in here:

  1. cider-switch-to-relevant-repl-buffer is mapped to C-c C-z. I've tidied related keybindings, and specially removed the ability to rotate and to display the current nrepl connection from the REPL mode (still exists for CIDER mode). It makes little sense to do it here, and generally I think it's good to prune where possible.

  2. cider-switch-to-relevant-repl-buffer again takes 2 prefix args (first one does the namespace switch, second one triggers IDO to select project).

  3. Switch will NOT be made based on project dir if there exists a REPL connection without a project dir. If this is so, then a message is printed.

  4. Better message output when a switch is made.

Some notes:

  1. I've kept cider-switch-to-repl-buffer around. This is because it's quite discreet in what it does and it does offer users an option if they dislike the funkier behavour. In time someone might remove it.

  2. I've tried to make "Managing multiple sessions" in the readme a little more easy going. There are issues around the precise terminology to use, like "CIDER minor mode" vs "Clojure buffer", or "current nrepl connection" vs "default nrepl connection", "switch vs select".

@@ -158,17 +158,44 @@ of the current source file."
(interactive "P")
(if (not (cider-connected-p))
(message "No active nREPL connection.")

Copy link
Member

Choose a reason for hiding this comment

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

Why are they blank lines in the code? In Lisps blank lines are generally considered bad style.

@bbatsov
Copy link
Member

bbatsov commented Jan 13, 2014

You've removed C-c M-d from the docs, but I don't think you removed it from the code.

@bbatsov
Copy link
Member

bbatsov commented Jan 13, 2014

My bad, just saw it now.

@bbatsov
Copy link
Member

bbatsov commented Jan 13, 2014

I wonder if it makes sense to make cider-switch-to-repl-buffer configurable. It could simply call-interactively whatever's the value of cider-switch-to-repl-command (a defcustom). It's not that people can't simply rebind C-c C-z to another command, but this seems like a cleaner approach.

@jonpither
Copy link
Contributor Author

Spaces removed and defcustom added.

underlying project directories:

```el
(setq cider-repl-tab-command 'cider-switch-to-current-repl-buffer)
Copy link
Member

Choose a reason for hiding this comment

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

This should be cider-switch-to-repl-command :-)

@jonpither
Copy link
Contributor Author

Yeah I just saw and fixed :-)


You can display the current nREPL connection using <kbd>C-c M-d</kbd> and rotate through available connections using <kbd>C-c M-r</kbd>.
To explicitly choose the REPL buffer that <kbd>C-c C-z</kbd> uses based on project directory, use a double prefix <kbd>C-u C-u C-c C-z</kbd>.
Copy link
Member

Choose a reason for hiding this comment

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

I guess you should mention somewhere, that this assumes you're using cider-switch-to-relevant-repl.

@jonpither
Copy link
Contributor Author

Fair enough I can fix that (although I think we're in extremely rare territory :-)).

For the sake of simplicity I'd like to have a single msg that applies to all ambiguous cases, right now it's:

"Could not determine relevant nREPL connection, using current...".

So I could add a check to cider-find-connection-buffer-for-project-directory that doesn't return a match if there is more than one possible matching repl connection.

@bbatsov
Copy link
Member

bbatsov commented Jan 13, 2014

Fair enough. You'll also have to rebase the final version of the branch against the current master.

@jonpither
Copy link
Contributor Author

Fair enough = put the extra check into cider-find-connection-buffer-for-project-directory?

@bbatsov
Copy link
Member

bbatsov commented Jan 13, 2014

Yes.

@jonpither
Copy link
Contributor Author

Changes made.

@jonpither
Copy link
Contributor Author

It's in good shape but lets merge this tomorrow, I'll review it all again in the am.

@jonpither
Copy link
Contributor Author

Ready for review/merge & couple of things:

  1. I've added behavour for another case - where there is only 1 connection and it's remote. In this case we shouldn't print a message saying "could not determine the relevant nREPL connection...". We should just switch.

  2. You might want to review the use of blank lines in the doc strings in cider-interaction.el, i.e. for cider-find-connection-buffer-for-project-directory. Don't know if this is desirable, there are blank lines in docstrings elsewhere I've noticed.

@jonpither
Copy link
Contributor Author

^ 1 last commit to make terser the msg "Switch to REPL". Done for now :-)

bbatsov added a commit that referenced this pull request Jan 14, 2014
Tidying up switch-to-relevant-buffer.
@bbatsov bbatsov merged commit 7e7bac3 into clojure-emacs:master Jan 14, 2014
@jonpither jonpither deleted the tidy branch January 14, 2014 10:27

CIDER maintains a list of nREPL connections and a single 'default' connection. When you execute CIDER commands in a Clojure editing buffer such as to compile a namespace, these commands are executed against the default connection.

You can display the default nREPL connection using <kbd>C-c M-d</kbd> and rotate the default connection using <kbd>C-c M-r</kbd>. Another option for setting the default connection is to execute the command <kbd>M-x nrepl-make-repl-connection-default</kbd> in the appropriate REPL buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

C-c M-d and C-c M-r have been removed right? Or have I missed something?

Copy link
Member

Choose a reason for hiding this comment

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

Only from cider-repl-mode. They're still available in cider-mode.

@vedang
Copy link
Contributor

vedang commented Jan 15, 2014

Just to add my experience, the project I'm working on has required me to connect to multiple repls on different machines as well as open 2 repls to the same project on my own machine :). I appreciate that I have the option to use the original cider-switch-to-repl which is easier to wrap my head around. I, for one, will certainly be using cider-switch-to-current-repl-buffer

@jonpither
Copy link
Contributor Author

@vedang where ambiguity exists cider-switch-to-relevant will behave the same as cider-switch-to-current, so for your use case there should be no difference apart from the message printed.

That said, I can imagine how you'd want the most bare bones switching behavour possible given your scenario.

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.

3 participants