[info] Add support for automatically downloading sources JARs

[alexander-yakushev]

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

Not a lot of changes were needed here. Given how things are implemented on the Orchard side, cider-nrepl only needs to set up a correct *download-sources-jar-fn* before calling out to Orchard functions that do Java sources discovery.

We maintain a ConcurrentHashMap (logically, a set, but there is no ConcurrentHashSet, and CHM API for this usecase is cleaner than (atom #{})) of Maven coordinates (groupId/artifactId "version") of the dependencies we already tried downloading sources once. If it succeeded, we won't call this function again anyway, if it failed, the CHM will prevent trying for the second time. This debouncer only exists in memory which I suppose makes it easier to retry (by restarting) if the sources failed to download because bad Internet or something, not because the sources JAR does not exist in the repository. If this cache was persistent, retrying will be a bit more involved. Anyway, this may change after user feedback; maybe, there would be frequent annoying cases where certain Java classes wouldn't have sources JAR, and it will be annoying that the first C-c C-d C-d on such class is slow after each REPL restart.

We bind *download-sources-jar-fn* only in info op, not the eldoc op. Eldoc is less intentional and much more common, so I don't want to make slow and download things. Counter to that, info is invoked by M-. or C-c C-d C-d where the user explicitly requested something, so it makes sense to attempt downloading then. Until the JAR is downloaded, eldoc will simply return results obtained via reflection, after that it will pick up the parsed Javadocs.

---

• You've added tests to cover your change(s)
• You've updated the README

[alexander-yakushev]

This initializer was removed in latest Orchard.

[alexander-yakushev]

I don't like that a library can System/exit, I think it takes too much upon itself. Throwing an exception is better because it can be caught (for whichever reason the user has), it can be properly logged, you get a stacktrace to know who required cider.nrepl namespace (e.g. this might be a surprise to the user), etc.

[bbatsov]

Fine by me - that System/exit was always controversial. Ideally we'd have at some point a way for the server to notify clients if they are not meeting some runtime requirements, but if cider-nrepl can work to some extent and the users are aware of this, it's probably a good trade-off.

[alexander-yakushev]

Just to clarify, if user doesn't try to catch the exception around requiring cider.nrepl, then the end result will be the same as in System/exit – the server will fail to start. It's just that throwing an exception gives more options to react to it and properly report.

[bbatsov]

Yeah, yeah - I get this. My point was mainly that the way the code was previously written it would fail on the version check alone, even if it might actually work in practice and might be fine with some users. (working, but not really supported) I think the purpose of the explicit exit here was to prevent such scenarios from happening.

I agree it's better for the clients to decide how to handle an error from a library that they're using.

[bbatsov]

Apart from the failing tests the PR looks good to me.

[bbatsov]

We bind download-sources-jar-fn only in info op, not the eldoc op. Eldoc is less intentional and much more common, so I don't want to make slow and download things. Counter to that, info is invoked by M-. or C-c C-d C-d where the user explicitly requested something, so it makes sense to attempt downloading then. Until the JAR is downloaded, eldoc will simply return results obtained via reflection, after that it will pick up the parsed Javadocs.

You might want to mention this somewhere in the comments and perhaps later in CIDER's docs, so people are aware this is intentional.

[alexander-yakushev]

You might want to mention this somewhere in the comments and perhaps later in CIDER's docs, so people are aware this is intentional.

I'll mention it in the CIDER docs; there's probably not a place to mention it here because it is not controlled on cider-nrepl side. If the client decides to set the :download-sources-jar key for eldoc, they can do that, cider-nrepl will not object.