Skip to content

Add client-info to clone op #3806

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

Merged
merged 4 commits into from
Apr 10, 2025

Conversation

ericdallo
Copy link
Contributor

@ericdallo ericdallo commented Apr 9, 2025

As discussed on this thread, we want to have client information about the nrepl client, we discussed about adding this extra info in clone op as optional information.

Related to nrepl/nrepl#370


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)

Thanks!

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

@ericdallo ericdallo changed the title [] Add client-info to clone op Add client-info to clone op Apr 9, 2025
@ericdallo ericdallo marked this pull request as ready for review April 9, 2025 13:15
@ericdallo
Copy link
Contributor Author

@bbatsov I think the tests failing are not related to this PR

@alexander-yakushev
Copy link
Member

Looks like it is related to the PR:
image

@ericdallo
Copy link
Contributor Author

That's weird, the PR just add optional data, any clues @alexander-yakushev ?

@alexander-yakushev
Copy link
Member

Yeah, it is weird. This works correctly locally. @bbatsov any ideas?

@bbatsov
Copy link
Member

bbatsov commented Apr 10, 2025

There's a mocked nREPL server for the integration tests that you'll probably have to update. Guess something's off with it's implementation.

@bbatsov
Copy link
Member

bbatsov commented Apr 10, 2025

@alexander-yakushev
Copy link
Member

Looks like the mock expects the clone message to be exactly this, without the extra fields: https://github.com/clojure-emacs/cider/blob/master/test/nrepl-server-mock.el#L46

@ericdallo
Copy link
Contributor Author

makes sense, just wondering how to get civer-version on that file

@ericdallo
Copy link
Contributor Author

I think I got it :)

@alexander-yakushev
Copy link
Member

LGTM!

@alexander-yakushev alexander-yakushev merged commit 2c855f9 into clojure-emacs:master Apr 10, 2025
16 checks passed
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