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

[info] Add support for sources JAR downloading on `info' events #3769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Jan 5, 2025

Sister PRs: clojure-emacs/orchard#310, clojure-emacs/cider-nrepl#911.

This is very raw, I need help with the implementation and with the code style and, probably, a test would be helpful.

  1. New customizable cider-download-sources-jars to turn on/off the whole thing. Disabled for now, maybe enabled by default some time in the future? Gotta see the feedback.
  2. I had to modify the core nrepl-send-sync-request so that it can optionally accept a callback function to invoke on the received messages. This is because info op can now return an interim response download-sources-jar telling the client that it performs the downloading right now and it can take a while. The callback allows to react to this and show user a message while the op itself is still synchronous and waits for done. The message hint looks like this:
    Local source not found, downloading Java sources for artifact org.clojure/clojure 1.12.0...
    
    I welcome suggestions about the wording.
  3. Build tool for the project is currently a placeholder. Do we track somewhere which build tool jack-in used to start the REPL? Damn, now that I thought of it, jack-in is not the only way to start the REPL. Perhaps, I need to rethink the approach and try to figure out the current REPL's build tool on the Clojure side, in either cider-nrepl or Orchard. Moved build tool discovery to Orchard.

Anyway, I'm showing this for some initial eyeballing.

More questions:


  • You've added tests (if possible) to cover your change(s)
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@bbatsov
Copy link
Member

bbatsov commented Jan 6, 2025

I had to modify the core nrepl-send-sync-request so that it can optionally accept a callback function to invoke on the received messages. This is because info op can now return an interim response download-sources-jar telling the client that it performs the downloading right now and it can take a while. The callback allows to react to this and show user a message while the op itself is still synchronous and waits for done. The message hint looks like this:

We might have to think about this part, as callbacks and sync requests don't mix very well in my brain. I'm wondering if it won't be better (easier?) to handle the absence of the source transparently - until it's downloaded we just treat it as missing. Anyways, I'll have to think about our options here, as off the top of my head every idea comes with some issues - e.g. we can use async calls for info, but it will complicate the interplay with some Emacs APIs that were not designed to used in such a way.

@alexander-yakushev
Copy link
Member Author

How would you suggest the downloading happen? Start it in the background but tell user that the sources are missing and they maybe will be present on the next request?

I understand your concern about mixing sync and async behavior. Here the only action that async does is notifying the user, no control logic. I supposed it should be fine like this.

Turning 'info' completely async is an option but not a very good one, I think. The doc popup or jump-to-def might happen a few seconds in the future when the user has already given up waiting and proceeded with their work.

@bbatsov
Copy link
Member

bbatsov commented Jan 6, 2025

How would you suggest the downloading happen? Start it in the background but tell user that the sources are missing and they maybe will be present on the next request?

Yeah, that's what I was thinking about, but admittedly it's not a great option.

I understand your concern about mixing sync and async behavior. Here the only action that async does is notifying the user, no control logic. I supposed it should be fine like this.

Yeah, it will be fine. My main concern is just that it's not very obvious how a callback operates in the sync context. Perhaps it will be enough to just elaborate in the docstring on this and perhaps caution people to avoid doing so, unless they really know what they are doing.

Turning 'info' completely async is an option but not a very good one, I think. The doc popup or jump-to-def might happen a few seconds in the future when the user has already given up waiting and proceeded with their work.

Fair point.

@@ -342,6 +346,17 @@ The default value in nREPL is 1024."
:group 'cider
:package-version '(cider . "0.25.0"))

(defcustom cider-download-sources-jars nil
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps cider-download-java-sources would be a bit better name here?

You'll also want to mention this defcustom somewhere in the docs.

Copy link
Member

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

Apart from my small remarks above and inline - I'm fine with the general shape of the proposal.

@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Jan 6, 2025

Perhaps it will be enough to just elaborate in the docstring on this and perhaps caution people to avoid doing so, unless they really know what they are doing.

This is fair. I'll expand the docstring to emphasize that the callback in a sync request shouldn't be used for logic control. It's indeed sort of a hack.

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