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

Allow starting the sideloader for the tooling session #3039

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Aug 31, 2021

CIDER's deferred loading of middleware means that requires can happen during all
kinds of nREPL ops, many of which are performed over the tooling session. To
make this work via the sideloader we need it to be active both on the regular
session and on the tooling session, so provide a flag for starting it in either
session.

Part of #3037


  • 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)

CIDER's deferred loading of middleware means that requires can happen during all
kinds of nREPL ops, many of which are performed over the tooling session. To
make this work via the sideloader we need it to be active both on the regular
session and on the tooling session, so provide a flag for starting it in either
session.
@plexus plexus force-pushed the sideloader-in-tooling-session branch from eb335cb to ca6414d Compare August 31, 2021 09:24
@plexus
Copy link
Contributor Author

plexus commented Aug 31, 2021

I originally had the op automatically go to both sessions, the more granular version seems preferable though. Not sure if it really makes sense to add tests here or doc updates.

@@ -162,13 +162,14 @@ Signal an error if it is not supported."
(unless (cider-nrepl-op-supported-p op)
(user-error "`%s' requires the nREPL op \"%s\" (provided by cider-nrepl)" this-command op)))

(defun cider-nrepl-send-request (request callback &optional connection)
(defun cider-nrepl-send-request (request callback &optional connection tooling)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a more generic session parameter wouldn't be more appropriate here. Technically a connection can have other sessions than the two created by CIDER by default, and this would allow us to interact with them. I guess this will require a small change in nrepl-send-request as well, though.

Copy link
Member

Choose a reason for hiding this comment

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

Anyways, I can just do this myself down the road. Let's keep your change focused to the problem at hand.

@bbatsov
Copy link
Member

bbatsov commented Aug 31, 2021

I don't think tests/doc updates for this are needed. I agree it makes sense for the tooling session to enable sideloading as well.

@bbatsov bbatsov merged commit d39ff45 into clojure-emacs:master Aug 31, 2021
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.

2 participants