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

Discover nbb through nrepl describe #3466 #3467

Conversation

benjamin-asdf
Copy link
Contributor

@benjamin-asdf benjamin-asdf commented Sep 16, 2023

Fixes #3466

See issue.

The main idea for the dynamic discovery of the nrepl server capabilities was that you start out with a blank repl
and it augments itself by discovering the server.

The eval during startup was a hit of 1.6 seconds on my machine and 2 for vemv.

  1. This check was currently only done for nbb connections (when connecting with cider-connect-clj)
  2. For nbb, we do have a version from the describe op, so we already can say if it is nbb, or not

So the current change is to rely on the nrepl descibe op.


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

Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

Thanks much for responding so quickly!

A couple suggestions. Also, please also include a Changelog entry.

cider-connection.el Show resolved Hide resolved
cider-connection.el Show resolved Hide resolved
@benjamin-asdf benjamin-asdf requested a review from vemv September 16, 2023 17:53
Copy link
Member

@vemv vemv left a comment

Choose a reason for hiding this comment

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

LGTM!

Please make sure to make the linter pass.

CHANGELOG.md Outdated Show resolved Hide resolved
vemv pushed a commit that referenced this pull request Sep 17, 2023
@vemv vemv closed this in b75e538 Sep 17, 2023
@vemv
Copy link
Member

vemv commented Sep 17, 2023

Thanks much!

I've cherry-picked this, preserving attribution.

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.

Simplify nbb-related check
2 participants