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

Detect the repl type in cider-sync-request:info based on the current buffer #2560

Merged
merged 2 commits into from
Jan 14, 2019

Conversation

ak-coram
Copy link
Contributor

@ak-coram ak-coram commented Jan 7, 2019

cider-sync-request:info is using cider-nrepl-send-sync-request without a connection argument, which leads to (cider-current-repl 'any) being used to determine the current connection. This means that when M-. (cider-find-var) is used in a cljs buffer it still sometimes tries to (unsuccessfully) find vars via the clj session of the project. I've added an explicit connection argument to the cider-nrepl-send-sync-request call in cider-sync-request:info using (cider-current-repl) (which determines the repl type based on the current buffer). This resolved the issue for me.


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 (make test)
  • All code passes the linter (make 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)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

@ak-coram
Copy link
Contributor Author

ak-coram commented Jan 8, 2019

I've noticed that cider-sync-request:eldoc has the same problem because nil is passed as the connection. I can add another commit for that as well if you'd like.

@ak-coram
Copy link
Contributor Author

ak-coram commented Jan 8, 2019

Probably the same also goes for cider-sync-request:complete and possibly several other functions, but I'm not really sure about the ones related to namespaces (and the ones for specs).

@bbatsov
Copy link
Member

bbatsov commented Jan 8, 2019

I recall we discussed something like this with @kommen recently, but I'm fuzzy on the details and whether we actually decided on anything. I agree that picking some random connection is not the best way to go, and what you suggest seems reasonable.

I've noticed that cider-sync-request:eldoc has the same problem because nil is passed as the connection. I can add another commit for that as well if you'd like.

Yeah, that's weird. In such cases nREPL will create some transient session for the duration of the request.

@kommen
Copy link
Contributor

kommen commented Jan 8, 2019

This is the PR where we discussed this: #2467

I think we we came to the solution we have right now because of this comment:

Looking at the other code in the file I think of two things:
what happens if you only have a cljs connection? Seems to me the formatting should still work, as it's not really clj or cljs specific

#2467 (comment)

@ak-coram
Copy link
Contributor Author

ak-coram commented Jan 8, 2019

Instead of having to figure this out for each request type, maybe having cider-current-repl prefer repls of type (cider-repl-type-for-buffer) when called with 'any would be the easiest solution. It could still pick the most recent one available, but if there are any with the current buffer's repl type then pick the most recent one of those. I'll gladly update this PR if you prefer this solution.

@bbatsov
Copy link
Member

bbatsov commented Jan 12, 2019

That seems somewhat reasonable. Keep in mind that relatively few ops have ClojureScript specific versions, so we should probably focus only on those. I'd aim to replicate whatever dispatch logic we're using for eval, as the most fundamental op.

Also keep in mind that the terms clojure and clojurescript connection are a misnomer - all connections are actually the same, the "clojurescript" connections simply use an nREPL session where Piggieback is present. If you pass the right nREPL session to an operation it would work regardless of the connection used. The ClojureScript connection simply has as its session this piggieback nREPL session. Hopefully that's not super confusing. See also https://nrepl.org/nrepl/design/middleware.html#_sessions

@bbatsov
Copy link
Member

bbatsov commented Jan 12, 2019

To summarize again - we need some more specific routing only for ops that have ClojureScript versions. Currently I think those are only "eval" (which should have it done right by now), "complete" and "info".

@bbatsov
Copy link
Member

bbatsov commented Jan 12, 2019

Also - rebase on top of master to fix the build error.

@ak-coram ak-coram force-pushed the find-var-detect-repl-type branch from c7ce81a to 24c9316 Compare January 14, 2019 09:42
@ak-coram
Copy link
Contributor Author

I've rebased on top of master and added another commit for the
"complete" and "eldoc" ops (as eldoc isn't working for me reliably
without this change either).

@bbatsov bbatsov merged commit c2bbfb1 into clojure-emacs:master Jan 14, 2019
@kommen
Copy link
Contributor

kommen commented Jan 14, 2019

Thank you @ak-coram!

@ak-coram ak-coram deleted the find-var-detect-repl-type branch January 15, 2019 08:19
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