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

Add cider-load-buffer-and-switch-to-repl-buffer #1232

Merged
merged 1 commit into from
Sep 20, 2015

Conversation

jeremyheiler
Copy link
Contributor

@SirSkidmore and I wrote a command C-c M-z to abbreviate:

C-c C-k
C-u C-c C-z

Specifically, the function will load (eval) the current buffer, change the namespace of the REPL buffer to match the currently visited source file, and then switch to the relevant REPL buffer.

We thought about parameterizing the the function to selecting the REPL buffer, but that would have to be done before cider-load-buffer is invoked, and weren't sure how to handle that. While attempting to do it, the message from cider-load-buffer would be in the way unless you know to type something to get rid of it.

@Malabarba
Copy link
Member

This behavior would make a lot of sense to me under C-u C-c C-k, given how tightly related it is to C-c C-k.
Code looks good. 👍

@jeremyheiler
Copy link
Contributor Author

We thought through the command sequence, and we've concluded that the C-c M-z family makes the most sense if we update it to behave just like C-c C-z, but have it always load the buffer.

        C-c C-z =                                               switch to repl buffer
    C-u C-c C-z =                               load namespace, switch to repl buffer
C-u C-u C-c C-z =              prompt for repl, load namespace, switch to repl buffer

        C-c M-z = load buffer,                                  switch to repl buffer
    C-u C-c M-z = load buffer,                  load namespace, switch to repl buffer
C-u C-u C-c M-z = load buffer, prompt for repl, load namespace, switch to repl buffer

Using C-c C-k could work, but isn't as symmetrical with regard to C-c C-z.

        C-c C-k = load buffer
    C-u C-c C-k = load buffer,                  load namespace  switch to repl buffer
C-u C-u C-c C-k = load buffer, prompt for repl, load namespace, switch to repl buffer

@jeremyheiler
Copy link
Contributor Author

I have updated the function to accept universal arguments a la C-c C-z.

However, when running C-u C-u C-c M-z, the minibuffer shows the Loading %s... message from cider-load-file on top of the prompt from cider-set-relevant-connection. Is it possible to suppress the message, or have the prompt force its way to the front?

@expez
Copy link
Member

expez commented Jul 31, 2015

This behavior would make a lot of sense to me under C-u C-c C-k

👍

This also reduces the cognitive load of yet another keybinding, but just barely :/

@Malabarba
Copy link
Member

        C-c M-z = load buffer, switch to repl buffer

If I'm loading a file and switching to the REPL as a single action, I almost certainly want the namespace to be loaded on the REPL.

    C-u C-c M-z = load buffer, load namespace, switch to repl buffer

This looks much more useful than the other one.

Using C-c C-k could work, but isn't as symmetrical with regard to C-c C-z.

It's good to maintain symmetry where we can, but limiting the number of keybinds is important too. Cider is already starting to run out of these. ;-)

I like this proposal:

        C-c C-k = load buffer
    C-u C-c C-k = load buffer,                  load namespace  switch to repl buffer
C-u C-u C-c C-k = load buffer, prompt for repl, load namespace, switch to repl buffer

@jeremyheiler
Copy link
Contributor Author

If I'm loading a file and switching to the REPL as a single action, I almost certainly want the namespace to be loaded on the REPL.

A counter point: if you're working in user or some other namespace you've already loaded, and want to load the code from a different namespace that it already has required, and switch to the REPL in one action. Also, if you're working in user, theirs the benefit of having clojure.repl automatically referred.

I agree the more common case should be shorter. However, it isn't with C-u C-c C-k. The only benefit is that it's based off an existing command.

When considering C-u C-c C-k, it felt that it overcomplicated cider-load-buffer by adding the REPL and namespace switching. Also, the interactive part would have to consider that buffer is an optional argument. However, that could be easy to do; I'm not sure.

@@ -814,6 +814,8 @@ Keyboard shortcut | Description
<kbd>C-c M-n</kbd> | Switch the namespace of the REPL buffer to the namespace of the current buffer.
<kbd>C-c C-z</kbd> | Switch to the relevant REPL buffer. Use a prefix argument to change the namespace of the REPL buffer to match the currently visited source file.
<kbd>C-u C-u C-c C-z</kbd> | Switch to the REPL buffer based on a user prompt for a directory.
<kbd>C-c M-z</kbd> | Load (eval) the current buffer and switch to the relevant REPL buffer. Use a prefix argument to change the namespace of the REPL buffer to match the currently visited source file.
<kbd>C-u C-u C-c M-z</kbd> | Load (eval) the current buffer and switch to the REPL buffer based on a user prompt for a directory.
Copy link
Member

Choose a reason for hiding this comment

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

Two prefix args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment above: #1232 (comment)

@bbatsov
Copy link
Member

bbatsov commented Aug 2, 2015

I'm fine with the idea, but I'm not sure what the default behaviour should be. Seems there might be some point in switching the ns by default, but on the other hand this won't be consistent with the current go-to-repl behaviour.

@Malabarba
Copy link
Member

I should probably say that I don't use the REPL buffer all that much, so take my opinion with a grain of salt. :)
In any case, I don't oppose to any of the options.

@bbatsov
Copy link
Member

bbatsov commented Aug 9, 2015

Looking at the code behind C-c C-z, the variant with selecting manually a project looks pretty strange to me. I totally don't remember what was the use-case behind this and I'll probably remove it the moment #1217 is done.

@bbatsov
Copy link
Member

bbatsov commented Aug 11, 2015

OK, so C-u C-u C-c C-z is no more. You can update the PR to reflect this.

@jonpither
Copy link
Contributor

Background on C-u C-u C-z C-z is in #350, the origin living in Hugo's question of "What happens when there are multiple REPLs for a single project".

I would agree with simplifying it away, unless the original concern stands tall.

@bbatsov
Copy link
Member

bbatsov commented Aug 14, 2015

Background on C-u C-u C-z C-z is in #350, the origin living in Hugo's question of "What happens when there are multiple REPLs for a single project".

This was problematic before and will remain to be problematic now as well. Basically you'll just fallback to the default REPL, which seems reasonable to me.

@bbatsov
Copy link
Member

bbatsov commented Aug 17, 2015

One more thing - now all operations are auto-dispatched via the "correct" connection and fallback to the default connection if the connection associated with a project cannot be determined. There's still some room for improvement, but overall people will have to deal with connection switching way less.

@jonpither
Copy link
Contributor

Cool. That was a bit of an elephant in the room in that if Cider knows the 'relevant/correct' connection, then why not always use that info for all operations. It's a pretty big change to make as it's in the core flow. If there's no problems for anyone, then excellent!

@bbatsov
Copy link
Member

bbatsov commented Aug 17, 2015

Well, previous the biggest practical issue was that you could not associate a local project with a connection created with cider-connect. The problem with multiple connections sharing the same project still exists, although I do plan to add some subfolder project support which should solve this to some extent. The only unsolvable problem is what do if you have two connections to different hosts for the same project, but I think that's not pretty common.

@bbatsov
Copy link
Member

bbatsov commented Aug 31, 2015

@jeremyheiler ping

@jeremyheiler
Copy link
Contributor Author

@bbatsov Sorry for the delay. My son was born shortly after opening this PR. :-)

I should have some time this week to fix it up.

@bbatsov
Copy link
Member

bbatsov commented Aug 31, 2015

@bbatsov Sorry for the delay. My son was born shortly after opening this PR. :-)

Congrats!

@jeremyheiler
Copy link
Contributor Author

@bbatsov I've pushed some changes, and rebased on from master.

bbatsov added a commit that referenced this pull request Sep 20, 2015
Add cider-load-buffer-and-switch-to-repl-buffer
@bbatsov bbatsov merged commit 1ce1875 into clojure-emacs:master Sep 20, 2015
@bbatsov
Copy link
Member

bbatsov commented Sep 20, 2015

👍

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.

None yet

5 participants