[Fix #1299] Eval in both REPLs for cljc and cljx

[expez]

This changes the load file request to dispatch to cider-other-connection in addition to cider-current-connection, when the file is a cljc or cljx file.

[expez]

The build result doesn't make sense to me. I'm not even sure what the difference between the two builds are.

[bbatsov]

There's just one shortcoming of the current implementation - the responses from the two requests will be interleaved, meaning most people will probably not realize what's going on. @Malabarba and I mentioned this yesterday as the biggest issue we need to tackle - how to handle the 2 return values that we'd get from operations like this one (or eval on multiple connections).

[bbatsov]

The build result doesn't make sense to me. I'm not even sure what the difference between the two builds are.

Which build result?

[Malabarba]

I think this whole diff should be implemented as a macro that checks for (cider--cljc-or-cljx-buffer-p). This can then be used on the already-merged namespace switching too. Here's an example of how it would work:

(cider-do-connections (conn)
  (cider-nrepl-request:eval (format "(in-ns '%s)" ns)
                            (cider-repl-switch-ns-handler conn)
                            conn))

This will make it easy for us to customize this behavior, implement a toggle variable, etc.

[Malabarba]

Ok. I like this feature more than I initially did. I think dispatching load-file to both repls is a good default.
There are just 2 points I'd like to bring up:

1. There should be a macro (cider-do-connections?) that checks if we're in a cljc file and runs body once for each relevant connection (otherwise just run body for the current connection). See my comment here: https://github.com/clojure-emacs/cider/pull/1399/files#r44129568
2. Whenever we mess with stuff that refactors connections, things tend to break. I'd be slightly more comfortable if we merged this after the 0.10 release. But maybe I'm just being a coward. :-)

[bbatsov]

Whenever we mess with stuff that refactors connections, things tend to break. I'd be slightly more comfortable if we merged this after the 0.10 release. But maybe I'm just being a coward. :-)

There will definitely be some period of bug fixing after the initial release, so I wouldn't worry that much about this. At some point we'll have to invest into better tests for the connection-related functionality, though. :-)

[expez]

cider-do-connections is a good idea, but there's the issue of whether the body acts on a file or a buffer. In this pr we're dispatching differently based on a file but in the set-ns code, from yesterday, we were acting on buffers.

I don't think I'll have time to make these changes this week. If anyone else wants to take this across the finish line that would be great, if not I'll probably have some more time for CIDER next week.

I also think this needs a bit more manual testing.

[Malabarba]

cider-do-connections is a good idea, but there's the issue of whether the body acts on a file or a buffer.

I don't see why files vs buffers is a meaningful distinction here, however....

Once we starting doing this for local operations (eq, eval-last-sexp), the macro will have to take a keyword that indicates that we also care about local context. So we could also use a keyword to indicate we only care about the file extension, and not the major mode.

[expez]

[Stupid Artur stupidly erased this comment by accident.]

[expez]

This feature is now blocked on #1400

I can't test this because the middleware is constantly setting the repl type to the wrong value.

[Malabarba]

Oh god. I just realised I think I edited your comment instead of posting a comment. 😱sorry about that!
I've no idea how that happened. I'm pasting my comment here:

---

And that made sense in terms of set-ns because that function acts in the context of a buffer, but cider-load-file takes filename is an input argument and so does not.

I prefer to check for clojurec-mode even on the load-file operation and never manually check the filename. I think we should have just a single way of determining “is this a dual file/buffer?” and we shouldn't distinguish a buffer from the file it is associated with. (the dynamic dispatch only checks file extension because we didn't have clojurescript-mode back then, and I'd like to change that).

This is simpler, and makes it easier for the user to configure the behaviour. If the user has manually configured clojurec-mode to activate on files that don't have the .cljc extension, then they want this file to be treated as a .cljc file, and I don't want to second-guess them.

[expez]

Aight, once I can test this feature I will rewrite cider-load-file to work in terms of cider-load-buffer instead of the other way around. That way we can branch on the mode in the buffer and not worry about file extensions.

[bbatsov]

.cljc or .cljx

[bbatsov]

One small remark about the commit message title - it's a bit unclear in it which eval is being referred to there. Today for moment I decided you've implemented interactive eval operations for cljc files as well. :-)

[expez]

One small remark about the commit message title - it's a bit unclear in it which eval is being referred to there.

The current patch affects both C-c C-k and C-c C-l. We should definitely do the same for C-c C-c. I'm not sure about C-x C-e.

[bbatsov]

I'm not sure about C-x C-e.

Don't see why we shouldn't do it for it as well.

[expez]

Rewritten in terms of cider-do-connections

[expez]

What can I do about this gensym problem the linter is complaining about?

[Malabarba]

Use make-symbol instead.

[bbatsov]

Apart from my small remarks the code looks good to me.

[expez]

I can't test it due to #1400 but #1400 can't be reproduced. Perhaps someone else could test this so we could get it merged?

[bbatsov]

Just rebase this and let's have it merged. I doubt it will break anything.

[Malabarba]

There should still be a (car cider-connections) as a last clause here. Otherwise this function might return nil.

[Malabarba]

Don't merge yet. I tested it and it seems to be breaking the state tracker. @expez, do you have any local work on this, or can I rebase it to current master?

[expez]

Go ahead and rebase.

[Malabarba]

I've found what the problem was. The “connection” and “session” specified in the load-file request didn't match (mostly because of -ensure-session), so everything got messed up.

Most relevantly, the state-tracker was sending information about the cljs session to the clj connection, which is why the repl-type info went wrong.

Overall, I think we should start purging current-session from our code as well. Due to our current structure, it is redundant information at best, and a bug source at worst.

[Malabarba]

I changed do-connections to a function, map-connections.

Sadly, we don't yet live in a world where inter-file macros are super
safe, so this is going to cause the least headache in the future.

[Malabarba]

The tests are failling due to byte-comp warnings from the new message-logging.
I was going to fix it myself but I'm not even sure what needs to be done (because it's catching a real bug) so I'll leave it to you @bbatsov. The issue is that some places call nrepl-log-message with a single argument (and worse, this argument is a string, not a request or a response so this code is guaranteed to barf when it's reached).

[bbatsov]

I was going to fix it myself but I'm not even sure what needs to be done (because it's catching a real bug) so I'll leave it to you @bbatsov.

Damn, I totally forgot about this. I know what you're referring to - an odd usage of the logging regarding some errors.

[bbatsov]

I've found what the problem was. The “connection” and “session” specified in the load-file request didn't match (mostly because of -ensure-session), so everything got messed up.

Most relevantly, the state-tracker was sending information about the cljs session to the clj connection, which is why the repl-type info went wrong.

Overall, I think we should start purging current-session from our code as well. Due to our current structure, it is redundant information at best, and a bug source at worst.

Purge it how? It's appended requests that don't have it, but there you can't automatically have there the tooling session if you want to use it. I was thinking lately because of the middleware exceptions that maybe the middleware requests should operate on the tooling session.

[bbatsov]

Ah, looking at the change you've made we can remove most usages of it I guess. That was my idea with appending the session automatically actually, but I guess it makes more sense to do so where the connection is explicitly known.

[Malabarba]

Yes. My thoughts were to just replace current-connection with nil everywhere, so we could still specify a connection if we ever want the tooling connection.
But I was a little scared of doing a wide refactor right before release. So I opted to make the minimal change that fixed the bug.

[bbatsov]

My thoughts were to just replace current-connection with nil everywhere, so we could still specify a connection if we ever want the tooling connection.

You meant current-session and current-tooling-session I guess. :-)

Yeah, let's focus on fixing everything else that's important right now. I addressed the failing build in the morning and will try to refine/update the documentation in time for the release. As the next two days will be busy for me at work I doubt I'll be able to work or more bug fixes/features before the release.

[expez]

Thanks for taking this one across the finish line @Malabarba!