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

jack-in support for the Basilisp Clojure dialect in Python #3683

Merged
merged 3 commits into from
May 29, 2024

Conversation

ikappaki
Copy link
Contributor

Hi,

can you please consider merging this patch to support Basilisp jack-in in CIDER. it resolves #3682.

I have added added an integration test and updated the documentation accordingly.

Currently the integration test currently fails on MS-Windows and has been temporarily disabled. This issue is addressed by basilisp-lang/basilisp#865 but the fix has not been released yet in PyPi. The problem is related flushing the stdout after a println. It only fails in CI, but works fine when invoked from a user's session.

Additionally, it requires clojure-mode v5.19 to support recognizing Basilisp's .lpy file extension

Thanks


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.

cider.el Outdated Show resolved Hide resolved
cider.el Outdated Show resolved Hide resolved
cider.el Outdated Show resolved Hide resolved
@bbatsov
Copy link
Member

bbatsov commented May 27, 2024

The PR looks good to me overall, sans my small remarks.

Does Basilisp implement the nREPL protocol fully? I'm wondering if there are some caveats that need to be documented if some ops are not implemented.

@ikappaki
Copy link
Contributor Author

The PR looks good to me overall, sans my small remarks.

removed obsolete option.

Does Basilisp implement the nREPL protocol fully? I'm wondering if there are some caveats that need to be documented if some ops are not implemented.

The Basilisp nREPL server currently implements the followings ops: eval, describe, info, eldoc, clone, close, load-file and complete, which cover the fundamental ops in my opinion. If you believe there are any caveats that need to be documented, please let me know.

@ikappaki
Copy link
Contributor Author

ikappaki commented May 27, 2024

I've just realized that jacking in on MS-Windows won't work even in a user session (I left a flush in my modified Basilisp package while testing), contrary to what I mentioned earlier. I think it's better to wait until the new Basilisp release with the fix is out before merging ...

Currently the integration test currently fails on MS-Windows and has been temporarily disabled. This issue is addressed by basilisp-lang/basilisp#865 but the fix has not been released yet in PyPi. The problem is related flushing the stdout after a println. It only fails in CI, but works fine when invoked from a user's session.

@bbatsov
Copy link
Member

bbatsov commented May 27, 2024

The Basilisp nREPL server currently implements the followings ops: eval, describe, info, eldoc, clone, close, load-file and complete, which cover the fundamental ops in my opinion. If you believe there are any caveats that need to be documented, please let me know.

I'm guessing you mean completions and lookup, not complete and info, right? Not a big difference for CIDER, but the former are nREPL ops and the latter are cider-nrepl ops, which served as the basis for the nREPL ops. See https://metaredux.com/posts/2020/06/15/nrepl-0-8-evolving-the-protocol.html for details.

@bbatsov bbatsov marked this pull request as draft May 27, 2024 12:47
@ikappaki ikappaki marked this pull request as ready for review May 28, 2024 18:30
@ikappaki
Copy link
Contributor Author

Hi @bbatsov,

I've checked in an update referencing the latest basilisp version with the recently released fix (thanks @chrisrink10). All tests now pass.

The Basilisp nREPL server currently implements the followings ops: eval, describe, info, eldoc, clone, close, load-file and complete, which cover the fundamental ops in my opinion. If you believe there are any caveats that need to be documented, please let me know.

I'm guessing you mean completions and lookup, not complete and info, right? Not a big difference for CIDER, but the former are nREPL ops and the latter are cider-nrepl ops, which served as the basis for the nREPL ops. See https://metaredux.com/posts/2020/06/15/nrepl-0-8-evolving-the-protocol.html for details.

I failed to mention earlier that the nREPL server is a port of the nbb nREPL server. I've kept the same ops, minus the macroexpand (which I couldn't find a use for at the time) and classpath which I plan to follow up at some point.

I haven't looked at the standard supported ops, but I'm reluctant of remove or renaming anything at this stage in fear I might break Calva support. I'll clean it up in the next basilisp nREPL update.

By the way, the complete keyword does appear to be used by CIDER (not sure if you meant it is not).

thanks

(-->
  id         "3"
  op         "describe"
  session    "b8a83d6d-84ee-4ab3-a1c0-92cd548634cf"
)
(<--
  id         "3"
  session    "b8a83d6d-84ee-4ab3-a1c0-92cd548634cf"
  ops        (dict
               clone     (dict)
               close     (dict)
               complete  (dict)
               describe  (dict)
               eldoc     (dict)
               eval      (dict)
               info      (dict)
               load-file (dict))
  status     ("done")
  versions   (dict
               basilisp (dict
                          incremental    "1"
                          major          "0"
                          minor          "."
                          version-string "0...1...0.b.2")
               python   (dict
                          incremental    "1"
                          major          "3"
                          minor          "12"
                          version-string "3.12.1 (tags/v3.12.1:2305ca5, Dec  7 2023, 22:03:25) [MSC v.1937 64 bit (AMD64)]"))
)
(-->
  id                        "10"
  op                        "complete"
  session                   "b8a83d6d-84ee-4ab3-a1c0-92cd548634cf"
  context                   "nil"
  enhanced-cljs-completion? "t"
  ns                        "user"
  prefix                    #("printl" 0 1 (fontified t) 1 2 (fontified t) 2 3 (fontified t) 3 4 (fontified t) 4 5 (fontified t) 5 6 (font-lock-multiline t fontified t))
)
(<--
  id          "10"
  session     "b8a83d6d-84ee-4ab3-a1c0-92cd548634cf"
  completions ((dict "candidate" "println" "ns" "basilisp.core" "type" "function")
 (dict "candidate" "println-str" "ns" "basilisp.core" "type" "function"))
  status      ("done")
)
(-->
  id         "11"
  op         "eldoc"
  session    "b8a83d6d-84ee-4ab3-a1c0-92cd548634cf"
  context    "nil"
  ns         "user"
  sym        "println"
)
(<--
  id         "11"
  session    "b8a83d6d-84ee-4ab3-a1c0-92cd548634cf"
  docstring  "Print the arguments to the stream bound to :lpy:var:`*out*` ..."
  eldoc      (nil
 ("x")
 ("x" "&" "args"))

  name       "println"
  ns         "basilisp.core"
  status     ("done")
  status     ("done")
  type       "function"
)

@bbatsov
Copy link
Member

bbatsov commented May 29, 2024

By the way, the complete keyword does appear to be used by CIDER (not sure if you meant it is not).

CIDER users both it's own ops (if present) and the standard nREPL ops if they are not. In general I think it's best for most nREPL implementations to stick to the official ops for completions and lookup, as this means they would work with more clients. I'm reasonably sure Calva supports completions and lookup, but I've never used it, so I can't be sure. Anyways, that's not a big deal for CIDER, just a general remark.

I failed to mention earlier that the nREPL server is a port of the nbb nREPL server. I've kept the same ops, minus the macroexpand (which I couldn't find a use for at the time) and classpath which I plan to follow up at some point.

nbb supports some built-in and some additional ops and that's fine. I've meant for a while to rename the CIDER ops to something like cider/complete and cider/info, so it's easier to figure out what's part of the protocol and what's a CIDER extension, but I never got to doing so.

@bbatsov bbatsov merged commit a74dff9 into clojure-emacs:master May 29, 2024
39 checks passed
@ikappaki
Copy link
Contributor Author

ikappaki commented Jun 3, 2024

Hi @bbatsov

CIDER users both it's own ops (if present) and the standard nREPL ops if they are not. In general I think it's best for most nREPL implementations to stick to the official ops for completions and lookup, as this means they would work with more clients.

Is this the official list of operations supported by the nREPL protocol that servers should adhere to? https://nrepl.org/nrepl/ops.html

I've meant for a while to rename the CIDER ops to something like cider/complete and cider/info, so it's easier to figure out what's part of the protocol and what's a CIDER extension, but I never got to doing so.

That would be very helpful indeed.

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.

jack-in support for Basilisp
2 participants