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

Add scittle to runtime versions #3600

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

benjamin-asdf
Copy link
Contributor

Replace this placeholder text with a summary of the changes in your PR.
The more detailed you are, the better.


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)

@benjamin-asdf
Copy link
Contributor Author

Scittle nrepl was saying a version but we don't take it into account for runtime classification.

CHANGELOG.md Outdated
@@ -7,6 +7,7 @@
- [#3588](https://github.com/clojure-emacs/cider/issues/3588): Compatibility with pwsh 7.3 quoting rules.
- Bump the injected `enrich-classpath` to [1.19.0](https://github.com/clojure-emacs/enrich-classpath/compare/v1.18.6...v1.19.0).
- Bump the `parseedn` required version to 1.2.1.
- [#3600](https://github.com/clojure-emacs/cider/pull/3600) Fix scittle jack in when using `cider-jack-in-clj`
Copy link
Member

Choose a reason for hiding this comment

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

You're missing a : and a . here.

@@ -456,12 +457,19 @@ But helps us know if this is a nbb repl, or not."
(when nrepl-versions
(nrepl-dict-get nrepl-versions "nbb-nrepl"))))

(defun cider--scittle-nrepl-version ()
"Likewise for scittle. See `cider--nbb-nrepl-version'."
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather copy the docstring from nbb, so this can be read standalone.

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.

I'd appreciate understanding, does scittle have a nrepl server? Mind to share some links?

If so ideally it would bave the same capabilities as a bb-nrepl server, namely ns-list and classpath ops, for correct Sesman functioning.

@vemv
Copy link
Member

vemv commented Dec 22, 2023

You could also expand the user manual to show that this is possible, its use case, sample setup etc.

...It would also help us verify this works as intended - I've never used scittle but would gladly QA this branch

@vemv vemv marked this pull request as draft January 1, 2024 11:13
@bbatsov
Copy link
Member

bbatsov commented Jan 16, 2024

@benjamin-asdf ping :-)

@benjamin-asdf
Copy link
Contributor Author

benjamin-asdf commented Jan 19, 2024

Hi, the write up is here.
https://github.com/babashka/scittle/tree/main/doc/nrepl

The last section should not be neccessary because it is replaced by cljs discovery. The cljs discovery was broken for scittle and
is fixed by adding this scittle version.

I am honestly not 100% happy about this. I thought that we had something where the client checks cljs capability by itself.
But we removed this mainly because of the startup performance of doing a cljs eval or something?

The problem right now is that if somebody like Jack Rusher writes a new clojurescript repl, it won't work directly. And it does for calva, which is then pointed out usually :P

@vemv
Copy link
Member

vemv commented Jan 22, 2024

But we removed this mainly because of the startup performance of doing a cljs eval or something?

Please check the git logs as I don't immediately recall what that thing was named, or why it was removed.

The problem right now is that if somebody like Jack Rusher writes a new clojurescript repl, it won't work directly.

Not sure of how things came to that point (if they really are). Per:

(defun cider-runtime ()
  "Return the runtime of the nREPl server."
  (cond
   ((cider--clojure-version) 'clojure)
   ((cider--babashka-version) 'babashka)
   ((cider--nbb-nrepl-version) 'nbb)
   ((cider--scittle-nrepl-version) 'scittle)
   (t 'generic)))

...the only cljs repls that need a special identification happen to come from the same author. There's a nice variety of cljs repls that don't need that.

@bbatsov
Copy link
Member

bbatsov commented Feb 28, 2024

@benjamin-asdf Ping :-)

@benjamin-asdf
Copy link
Contributor Author

@bbatsov Jo I fixed the remaining lint error (I hope)

@benjamin-asdf benjamin-asdf marked this pull request as ready for review February 28, 2024 16:14
CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## master (unreleased)

- [#3600](https://github.com/clojure-emacs/cider/pull/3600): Fix scittle jack in when using `cider-jack-in-clj.
Copy link
Member

Choose a reason for hiding this comment

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

This should go under Bugs fixed or Changes. Also you missed the closing backtick.

@@ -457,12 +458,22 @@ But helps us know if this is a nbb repl, or not."
(when nrepl-versions
(nrepl-dict-get nrepl-versions "nbb-nrepl"))))

(defun cider--scittle-nrepl-version ()
"Retrieve the underlying connection's scittle version."
(with-current-buffer
Copy link
Member

Choose a reason for hiding this comment

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

I'd use here more compact indentation, as the code reads better this way - e.g. like in the function above for nbb.

@bbatsov
Copy link
Member

bbatsov commented Mar 5, 2024

I think you'll need to rebase on top of master, as the changelog seems outdated on this branch. I also left a few small remarks.

@benjamin-asdf
Copy link
Contributor Author

@bbatsov Cool, I fixed the remaining issues

@bbatsov bbatsov merged commit 23540fd into clojure-emacs:master Mar 8, 2024
35 checks passed
@bbatsov
Copy link
Member

bbatsov commented Mar 8, 2024

Thanks!

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